Merge pull request #11260 from dannon/username-requirement-fix

[21.01] Routing fixes for username path components, username validation fixes

lib/galaxy/webapps/galaxy/api/users.py

@@ -416,7 +416,7 @@ class UserAPIController(BaseAPIController, UsesTagsMixin, BaseUIController, Uses
         # Update email
         if 'email' in payload:
             email = payload.get('email')
-            message = self._validate_email(email) or validate_email(trans, email, user)
+            message = validate_email(trans, email, user)
             if message:
                 raise exceptions.RequestParameterInvalidException(message)
             if user.email != email:
@@ -441,7 +441,7 @@ class UserAPIController(BaseAPIController, UsesTagsMixin, BaseUIController, Uses
         # Update public name
         if 'username' in payload:
             username = payload.get('username')
-            message = self._validate_publicname(username) or validate_publicname(trans, username, user)
+            message = validate_publicname(trans, username, user)
             if message:
                 raise exceptions.RequestParameterInvalidException(message)
             if user.username != username:
@@ -581,19 +581,6 @@ class UserAPIController(BaseAPIController, UsesTagsMixin, BaseUIController, Uses
         else:
             raise exceptions.ObjectAttributeInvalidException("This type is not supported. Given object_type: %s" % object_type)
 
-    def _validate_email(self, email):
-        ''' Validate email and username using regex '''
-        if email == '' or not isinstance(email, str):
-            return 'Please provide your email address.'
-        if not re.match(r'^(([^<>()[\]\.,;:\s@"]+(\.[^<>()[\]\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$', email):
-            return 'Please provide your valid email address.'
-        if len(email) > 255:
-            return 'Email cannot be more than 255 characters in length.'
-
-    def _validate_publicname(self, username):
-        if not re.match(r'^[a-z0-9\-]{3,255}$', username):
-            return 'Public name must contain only lowercase letters, numbers and "-". It also has to be shorter than 255 characters but longer than 2.'
-
     @expose_api
     def get_password(self, trans, id, payload=None, **kwd):
         """

lib/galaxy/webapps/galaxy/buildapp.py

@@ -16,6 +16,7 @@ import galaxy.model.mapping
 import galaxy.web.framework
 import galaxy.webapps.base.webapp
 from galaxy import util
+from galaxy.security.validate_user_input import VALID_PUBLICNAME_SUB
 from galaxy.util import asbool
 from galaxy.util.properties import load_app_properties
 from galaxy.web.framework.middleware.batch import BatchMiddleware
@@ -90,12 +91,14 @@ def app_factory(global_conf, load_app_kwds=None, **kwargs):
     webapp.add_route('/display_application/{dataset_id}/{app_name}/{link_name}/{user_id}/{app_action}/{action_param}/{action_param_extra:.+?}',
                      controller='dataset', action='display_application', dataset_id=None, user_id=None,
                      app_name=None, link_name=None, app_action=None, action_param=None, action_param_extra=None)
-    webapp.add_route('/u/{username}/d/{slug}/{filename}', controller='dataset', action='display_by_username_and_slug', filename=None)
-    webapp.add_route('/u/{username}/p/{slug}', controller='page', action='display_by_username_and_slug')
-    webapp.add_route('/u/{username}/h/{slug}', controller='history', action='display_by_username_and_slug')
-    webapp.add_route('/u/{username}/w/{slug}', controller='workflow', action='display_by_username_and_slug')
-    webapp.add_route('/u/{username}/w/{slug}/{format}', controller='workflow', action='display_by_username_and_slug')
-    webapp.add_route('/u/{username}/v/{slug}', controller='visualization', action='display_by_username_and_slug')
+
+    USERNAME_REQS = {'username': VALID_PUBLICNAME_SUB.pattern}
+    webapp.add_route('/u/{username}/d/{slug}/{filename}', controller='dataset', action='display_by_username_and_slug', filename=None, requirements=USERNAME_REQS)
+    webapp.add_route('/u/{username}/p/{slug}', controller='page', action='display_by_username_and_slug', requirements=USERNAME_REQS)
+    webapp.add_route('/u/{username}/h/{slug}', controller='history', action='display_by_username_and_slug', requirements=USERNAME_REQS)
+    webapp.add_route('/u/{username}/w/{slug}', controller='workflow', action='display_by_username_and_slug', requirements=USERNAME_REQS)
+    webapp.add_route('/u/{username}/w/{slug}/{format}', controller='workflow', action='display_by_username_and_slug', requirements=USERNAME_REQS)
+    webapp.add_route('/u/{username}/v/{slug}', controller='visualization', action='display_by_username_and_slug', requirements=USERNAME_REQS)
 
     # TODO: Refactor above routes into external method to allow testing in
     # isolation as well.

test/unit/data/security/test_validate_user_input.py

@@ -1,12 +1,33 @@
-from galaxy.security.validate_user_input import extract_domain
+from galaxy.security.validate_user_input import (
+    extract_domain,
+    validate_email_str,
+    validate_publicname_str,
+)
 
 
 def test_extract_full_domain():
-    assert extract_domain('jack@foo.com') == 'foo.com'
-    assert extract_domain('jack@foo.bar.com') == 'foo.bar.com'
+    assert extract_domain("jack@foo.com") == "foo.com"
+    assert extract_domain("jack@foo.bar.com") == "foo.bar.com"
 
 
 def test_extract_base_domain():
     # Use case: ignore subdomains to filter out disposable email addresses
-    assert extract_domain('jack@foo.com', base_only=True) == 'foo.com'
-    assert extract_domain('jack@foo.bar.com', base_only=True) == 'bar.com'
+    assert extract_domain("jack@foo.com", base_only=True) == "foo.com"
+    assert extract_domain("jack@foo.bar.com", base_only=True) == "bar.com"
+
+
+def test_validate_username():
+    assert validate_publicname_str("testuser") == ""
+    assert validate_publicname_str("test.user") == ""
+    assert validate_publicname_str("test-user") == ""
+    assert validate_publicname_str("test@user") != ""
+    assert validate_publicname_str("test user") != ""
+
+
+def test_validate_email():
+    assert validate_email_str("test@foo.com") == ""
+    assert validate_email_str("test-dot.user@foo.com") == ""
+    assert validate_email_str("test@com") != ""
+    assert validate_email_str("@not-a-domain") != ""
+    too_long_email = "N" * 255 + "@foo.com"
+    assert validate_email_str(too_long_email) != ""