Browse Source

HUE-551. Support a wider set of username characters

The username rule is now defined centrally, because other places that
interact with usernames may need it. I went Debian and allow
"[^-:\s][^:\s]*". It only rejects whitespaces and colons. In addition,
the first character may not be a dash.
bc Wong 14 years ago
parent
commit
9dee048096

+ 2 - 1
apps/useradmin/src/useradmin/templates/edit_user.mako

@@ -14,6 +14,7 @@
 ## See the License for the specific language governing permissions and
 ## limitations under the License.
 <%namespace name="wrappers" file="header_footer.mako" />
+<% import urllib %>
 
   % if username:
     ${wrappers.head('Edit User: ' + username + ' -- Hue Users')}
@@ -30,7 +31,7 @@
         Create User 
       % endif
     </h1>
-    <form action="${action}" method="POST" class="jframe_padded">
+    <form action="${urllib.quote(action)}" method="POST" class="jframe_padded">
       <dl>
         <%def name="render_field(field)">
           <dt>${field.label_tag() | n}</dt>

+ 3 - 2
apps/useradmin/src/useradmin/templates/list_users.mako

@@ -14,6 +14,7 @@
 ## See the License for the specific language governing permissions and
 ## limitations under the License.
 <%namespace name="wrappers" file="header_footer.mako" />
+<% import urllib %>
 
 ${wrappers.head()}
     <div id="useradmin_userlist" class="view">
@@ -41,8 +42,8 @@ ${wrappers.head()}
               ${user.last_login.strftime('%c')}
             </td>
             <td>
-              <a title="Edit ${user.username}" class="edit frame_tip" href="${ url('useradmin.views.edit_user', username=user.username) }">Edit</a>
-              <a title="Delete ${user.username}" class="delete frame_tip confirm_and_post" alt="Are you sure you want to delete ${user.username}?" href="${ url('useradmin.views.delete_user', username=user.username) }">Delete</a>
+              <a title="Edit ${user.username}" class="edit frame_tip" href="${ url('useradmin.views.edit_user', username=urllib.quote(user.username)) }">Edit</a>
+              <a title="Delete ${user.username}" class="delete frame_tip confirm_and_post" alt="Are you sure you want to delete ${user.username}?" href="${ url('useradmin.views.delete_user', username=urllib.quote(user.username)) }">Delete</a>
             </td>
           </tr>
         % endfor

+ 45 - 21
apps/useradmin/src/useradmin/tests.py

@@ -21,6 +21,9 @@
 Tests for "user admin"
 """
 
+import HTMLParser
+import urllib
+
 from nose.tools import assert_true, assert_equal
 
 from desktop.lib.django_test_util import make_logged_in_client
@@ -32,7 +35,25 @@ def reset_all_users():
   for user in User.objects.all():
     user.delete()
 
+def _unescape(html):
+  return HTMLParser.HTMLParser().unescape(html)
+
+
+def test_invalid_username():
+  BAD_NAMES = ('-foo', 'foo:o', 'foo o', ' foo')
+
+  c = make_logged_in_client(username="test", is_superuser=True)
+
+  for bad_name in BAD_NAMES:
+    assert_true(c.get('/useradmin/new'))
+    response = c.post('/useradmin/new', dict(username=bad_name, password1="test", password2="test"))
+    assert_true('not allowed' in response.context["form"].errors['username'][0])
+
+
 def test_user_admin():
+  FUNNY_NAME = '~`!@#$%^&*()_-+={}[]|\;"<>?/,.'
+  FUNNY_NAME_QUOTED = urllib.quote(FUNNY_NAME)
+
   reset_all_users()
   c = make_logged_in_client(username="test", is_superuser=True)
 
@@ -84,48 +105,51 @@ def test_user_admin():
   assert_equal({ 'username': ["User with this Username already exists."]}, response.context["form"].errors)
 
   # Create a new regular user (for real)
-  response = c.post('/useradmin/new', dict(username="test_user_creation", password1="test", password2="test", is_active="True"))
+  response = c.post('/useradmin/new', dict(username=FUNNY_NAME,
+                                           password1="test",
+                                           password2="test",
+                                           is_active="True"))
   response = c.get('/useradmin/')
-  assert_true("test_user_creation" in response.content)
+  assert_true(FUNNY_NAME in _unescape(response.content))
   assert_true(len(response.context["users"]) > 1)
   assert_true("Hue Users" in response.content)
 
   # Check permissions by logging in as the new user
-  c_reg = make_logged_in_client(username="test_user_creation", password="test")
+  c_reg = make_logged_in_client(username=FUNNY_NAME, password="test")
   # Regular user should be able to modify oneself
-  response = c_reg.post('/useradmin/edit/test_user_creation',
-                        dict(username = "test_user_creation",
-                        first_name = "Hello",
-                        is_active = True))
-  response = c_reg.get('/useradmin/edit/test_user_creation')
+  response = c_reg.post('/useradmin/edit/%s' % (FUNNY_NAME_QUOTED,),
+                        dict(username = FUNNY_NAME,
+                             first_name = "Hello",
+                             is_active = True))
+  response = c_reg.get('/useradmin/edit/%s' % (FUNNY_NAME_QUOTED,))
   assert_equal("Hello", response.context["form"].instance.first_name)
   # Can't edit other people.
   response = c_reg.post("/useradmin/delete/test")
   assert_true("You must be a superuser" in response.content,
               "Regular user can't edit other people")
   # Regular user should not be able to self-promote to superuser
-  response = c_reg.post('/useradmin/edit/test_user_creation',
-                        dict(username = "test_user_creation",
-                        first_name = "OLÁ",
-                        is_superuser = True,
-                        is_active = True))
+  response = c_reg.post('/useradmin/edit/%s' % (FUNNY_NAME_QUOTED,),
+                        dict(username = FUNNY_NAME,
+                             first_name = "OLÁ",
+                             is_superuser = True,
+                             is_active = True))
   assert_true("You cannot" in response.content,
               "Regular users can't self-promote to superuser")
 
   # Revert to regular "test" user, that has superuser powers.
   c_su = make_logged_in_client()
-  # Inactivate "test_user_creation"
-  c_su.post('/useradmin/edit/test_user_creation',
-                        dict(username = "test_user_creation",
-                        first_name = "Hello",
-                        is_active = False))
-  # Now make sure "test_user_creation" can't log back in
-  response = c_reg.get('/useradmin/edit/test_user_creation')
+  # Inactivate FUNNY_NAME
+  c_su.post('/useradmin/edit/%s' % (FUNNY_NAME_QUOTED,),
+                        dict(username = FUNNY_NAME,
+                             first_name = "Hello",
+                             is_active = False))
+  # Now make sure FUNNY_NAME can't log back in
+  response = c_reg.get('/useradmin/edit/%s' % (FUNNY_NAME_QUOTED,))
   assert_true(response.status_code == 302 and "login" in response["location"],
               "Inactivated user gets redirected to login page")
 
   # Delete that regular user
-  response = c_su.post('/useradmin/delete/test_user_creation')
+  response = c_su.post('/useradmin/delete/%s' % (FUNNY_NAME_QUOTED,))
   assert_true("Hue Users" in response.content)
   # You shouldn't be able to create a user without a password
   response = c_su.post('/useradmin/new', dict(username="test"))

+ 5 - 2
apps/useradmin/src/useradmin/urls.py

@@ -16,10 +16,13 @@
 # limitations under the License.
 
 from django.conf.urls.defaults import patterns, url
+from desktop.lib.django_util import get_username_re_rule
+
+username_re = get_username_re_rule()
 
 urlpatterns = patterns('useradmin',
   url(r'^$', 'views.list_users'),
-  url(r'^edit/(?P<username>\w+)$', 'views.edit_user'),
+  url(r'^edit/(?P<username>%s)$' % (username_re,), 'views.edit_user'),
   url(r'^new$', 'views.edit_user', name="useradmin.new"),
-  url(r'^delete/(?P<username>\w+)$', 'views.delete_user'),
+  url(r'^delete/(?P<username>%s)$' % (username_re,), 'views.delete_user'),
 )

+ 7 - 4
apps/useradmin/src/useradmin/views.py

@@ -22,7 +22,7 @@ import django
 import threading
 from django import forms
 from django.contrib.auth.models import User
-from desktop.lib.django_util import render, PopupException
+from desktop.lib.django_util import get_username_re_rule, render, PopupException
 from django.core import urlresolvers
 
 __users_lock = threading.Lock()
@@ -59,9 +59,12 @@ class UserChangeForm(django.contrib.auth.forms.UserChangeForm):
   This is similar, but not quite the same as djagno.contrib.auth.forms.UserChangeForm
   and UserCreationForm.
   """
-  username = forms.RegexField(label="Username", max_length=30, regex=r'^[\w]+$',
-          help_text = "Required. 30 characters or fewer. Letters and digits only.",
-          error_messages = {'invalid': "This value may contain only letters and numbers."})
+  username = forms.RegexField(
+      label="Username",
+      max_length=30,
+      regex='^%s$' % (get_username_re_rule(),),
+      help_text = "Required. 30 characters or fewer. No whitespaces or colons.",
+      error_messages = {'invalid': "Whitespaces and ':' not allowed" })
   password1 = forms.CharField(label="Password", widget=forms.PasswordInput, required=False)
   password2 = forms.CharField(label="Password confirmation", widget=forms.PasswordInput, required=False)
 

+ 5 - 0
desktop/core/src/desktop/lib/django_util.py

@@ -42,6 +42,9 @@ import desktop.lib.thrift_util
 DJANGO = 'django'
 MAKO = 'mako'
 
+# This is what Debian allows. See chkname.c in shadow.
+USERNAME_RE_RULE = "[^-:\s][^:\s]*"
+
 class Encoder(simplejson.JSONEncoder):
   """
   Automatically encodes JSON for Django models and
@@ -61,6 +64,8 @@ class Encoder(simplejson.JSONEncoder):
 
     return simplejson.JSONEncoder.default(self, o)
 
+def get_username_re_rule():
+  return USERNAME_RE_RULE
 
 def login_notrequired(func):
   """A decorator for view functions to allow access without login"""