From 391d91ebfe60d08effe208adec4c5341ac352deb Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Fri, 3 Apr 2020 01:08:32 +0530 Subject: [PATCH 1/4] Assign random password in user manager if none specified --- lib/galaxy/authnz/custos_authnz.py | 1 - lib/galaxy/managers/users.py | 5 ++++- test/unit/managers/test_UserManager.py | 9 +++++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/authnz/custos_authnz.py b/lib/galaxy/authnz/custos_authnz.py index 165f6a5615a..1a2f86e7f15 100644 --- a/lib/galaxy/authnz/custos_authnz.py +++ b/lib/galaxy/authnz/custos_authnz.py @@ -108,7 +108,6 @@ class CustosAuthnz(IdentityProvider): raise Exception("There already exists a user with email %s. To associate this external login, you must first be logged in as that existing account." % email) else: user = trans.app.user_manager.create(email=email, username=username) - user.set_random_password() trans.sa_session.add(user) trans.sa_session.flush() custos_authnz_token = CustosAuthnzToken(user=user, diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index 9553698ee92..a4f3a72e1da 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -93,7 +93,10 @@ class UserManager(base.ModelManager, deletable.PurgableManagerMixin): """ self._error_on_duplicate_email(email) user = self.model_class(email=email) - user.set_password_cleartext(password) + if password: + user.set_password_cleartext(password) + else: + user.set_random_password() user.username = username if self.app.config.user_activation_on: user.active = False diff --git a/test/unit/managers/test_UserManager.py b/test/unit/managers/test_UserManager.py index b293d9ce0c7..4aa3a377814 100644 --- a/test/unit/managers/test_UserManager.py +++ b/test/unit/managers/test_UserManager.py @@ -185,6 +185,15 @@ class UserManagerTestCase(BaseTestCase): response = json.loads(controller.login(self.trans, payload={"login": user2.username, "password": default_password})) self.assertEqual(response["message"], "Success.") + def test_empty_password(self): + self.log("should be able to create a user with no password") + user = self.user_manager.create(email='user@nopassword.com', username='nopassword') + self.assertIsNotNone(user.id) + self.assertIsNotNone(user.password) + # should not be able to login with a null or empty password + self.assertFalse(check_password("", user.password)) + self.assertFalse(check_password(None, user.password)) + # ============================================================================= class UserSerializerTestCase(BaseTestCase): From 34a70e7e0b4e4cf9bb1188cfb04905bc56fe324f Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Sun, 24 May 2020 23:08:36 +0530 Subject: [PATCH 2/4] Refactor password validation --- lib/galaxy/model/__init__.py | 4 ++ lib/galaxy/security/passwords.py | 2 + lib/galaxy/security/validate_user_input.py | 54 +++++++++++++------ .../webapps/galaxy/controllers/admin.py | 8 +-- 4 files changed, 47 insertions(+), 21 deletions(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index d51b38cc251..a167c38c1ad 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -51,6 +51,7 @@ import galaxy.security.passwords import galaxy.util from galaxy.model.item_attrs import get_item_annotation_str, UsesAnnotations from galaxy.security import get_permitted_actions +from galaxy.security.validate_user_input import validate_password_str from galaxy.util import (directory_hash_id, ready_name_for_url, unicodify, unique_id) from galaxy.util.bunch import Bunch @@ -387,6 +388,9 @@ class User(Dictifiable, RepresentById): """ Set user password to the digest of `cleartext`. """ + message = validate_password_str(cleartext, cleartext) + if message: + raise Exception("Invalid password: %s" % message) if User.use_pbkdf2: self.password = galaxy.security.passwords.hash_password(cleartext) else: diff --git a/lib/galaxy/security/passwords.py b/lib/galaxy/security/passwords.py index 1ecd07217b7..1858f0afad2 100644 --- a/lib/galaxy/security/passwords.py +++ b/lib/galaxy/security/passwords.py @@ -22,6 +22,8 @@ def hash_password(password): """ Hash a password, currently will use the PBKDF2 scheme. """ + if password is None: + raise Exception("password cannot be None") return hash_password_PBKDF2(password) diff --git a/lib/galaxy/security/validate_user_input.py b/lib/galaxy/security/validate_user_input.py index ae5dc840910..d0952f4c879 100644 --- a/lib/galaxy/security/validate_user_input.py +++ b/lib/galaxy/security/validate_user_input.py @@ -26,17 +26,44 @@ FILL_CHAR = '-' PASSWORD_MIN_LEN = 6 -def validate_email(trans, email, user=None, check_dup=True, allow_empty=False): - """ - Validates the email format, also checks whether the domain is blacklisted in the disposable domains configuration. - """ +def validate_email_str(email): + """Validates a string containing an email address.""" message = '' - if (user and user.email == email) or (email == "" and allow_empty): - return message if not(VALID_EMAIL_RE.match(email)): message = "The format of the email address is not correct." elif len(email) > EMAIL_MAX_LEN: message = "Email address cannot be more than %d characters in length." % EMAIL_MAX_LEN + return message + + +def validate_password_str(password, confirm): + if password != confirm: + return "Passwords do not match." + if not password or len(password) < PASSWORD_MIN_LEN: + return "Use a password of at least %d characters." % PASSWORD_MIN_LEN + return "" + + +def validate_publicname_str(publicname): + """Validates a string containing a public username.""" + if len(publicname) < PUBLICNAME_MIN_LEN: + return "Public name must be at least %d characters in length." % (PUBLICNAME_MIN_LEN) + if len(publicname) > PUBLICNAME_MAX_LEN: + return "Public name cannot be more than %d characters in length." % (PUBLICNAME_MAX_LEN) + if not(VALID_PUBLICNAME_RE.match(publicname)): + return "Public name must contain only lower-case letters, numbers, '.', '_' and '-'." + return '' + + +def validate_email(trans, email, user=None, check_dup=True, allow_empty=False): + """ + Validates the email format, also checks whether the domain is blacklisted in the disposable domains configuration. + """ + if (user and user.email == email) or (email == "" and allow_empty): + return '' + message = validate_email_str(email) + if message: + pass elif check_dup and trans.sa_session.query(trans.app.model.User).filter(func.lower(trans.app.model.User.table.c.email) == email.lower()).first(): message = "User with email '%s' already exists." % email # If the blacklist is not empty filter out the disposable domains. @@ -56,12 +83,9 @@ def validate_publicname(trans, publicname, user=None): """ if user and user.username == publicname: return '' - if len(publicname) < PUBLICNAME_MIN_LEN: - return "Public name must be at least %d characters in length." % (PUBLICNAME_MIN_LEN) - if len(publicname) > PUBLICNAME_MAX_LEN: - return "Public name cannot be more than %d characters in length." % (PUBLICNAME_MAX_LEN) - if not(VALID_PUBLICNAME_RE.match(publicname)): - return "Public name must contain only lower-case letters, numbers, '.', '_' and '-'." + message = validate_publicname_str(publicname) + if message: + return message if trans.sa_session.query(trans.app.model.User).filter_by(username=publicname).first(): return "Public name is taken; please choose another." return '' @@ -82,8 +106,4 @@ def transform_publicname(publicname): def validate_password(trans, password, confirm): - if len(password) < PASSWORD_MIN_LEN: - return "Use a password of at least %d characters." % PASSWORD_MIN_LEN - if password != confirm: - return "Passwords do not match." - return "" + return validate_password_str(password, confirm) diff --git a/lib/galaxy/webapps/galaxy/controllers/admin.py b/lib/galaxy/webapps/galaxy/controllers/admin.py index 70472e83322..3ea74006c4b 100644 --- a/lib/galaxy/webapps/galaxy/controllers/admin.py +++ b/lib/galaxy/webapps/galaxy/controllers/admin.py @@ -17,6 +17,7 @@ from galaxy.actions.admin import AdminActions from galaxy.exceptions import ActionInputError, MessageException from galaxy.model import tool_shed_install as install_model from galaxy.tool_shed.util.repository_util import get_ids_of_tool_shed_repositories_being_installed +from galaxy.security.validate_user_input import validate_password from galaxy.util import ( nice_size, sanitize_text, @@ -1378,10 +1379,9 @@ class AdminGalaxy(controller.JSAppLauncher, AdminActions, UsesQuotaMixin, QuotaP else: password = payload.get('password') confirm = payload.get('confirm') - if len(password) < 6: - return self.message_exception(trans, 'Use a password of at least 6 characters.') - elif password != confirm: - return self.message_exception(trans, 'Passwords do not match.') + message = validate_password(trans, password, confirm) + if message: + return self.message_exception(trans, message) for user in users.values(): user.set_password_cleartext(password) trans.sa_session.add(user) From c9f29aa888fbc6a3e1cc821fdafd1a104d65e4fd Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Sun, 24 May 2020 23:09:08 +0530 Subject: [PATCH 3/4] Refactor toolshed password validation --- .../webapps/galaxy/controllers/admin.py | 2 +- lib/tool_shed/util/admin_util.py | 11 ++--- lib/tool_shed/webapp/model/__init__.py | 4 ++ .../create_user_with_api_key.py | 47 ++++--------------- 4 files changed, 17 insertions(+), 47 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/controllers/admin.py b/lib/galaxy/webapps/galaxy/controllers/admin.py index 3ea74006c4b..510272c445d 100644 --- a/lib/galaxy/webapps/galaxy/controllers/admin.py +++ b/lib/galaxy/webapps/galaxy/controllers/admin.py @@ -16,8 +16,8 @@ from galaxy import ( from galaxy.actions.admin import AdminActions from galaxy.exceptions import ActionInputError, MessageException from galaxy.model import tool_shed_install as install_model -from galaxy.tool_shed.util.repository_util import get_ids_of_tool_shed_repositories_being_installed from galaxy.security.validate_user_input import validate_password +from galaxy.tool_shed.util.repository_util import get_ids_of_tool_shed_repositories_being_installed from galaxy.util import ( nice_size, sanitize_text, diff --git a/lib/tool_shed/util/admin_util.py b/lib/tool_shed/util/admin_util.py index 3175d2b74e1..d9d43b3e054 100644 --- a/lib/tool_shed/util/admin_util.py +++ b/lib/tool_shed/util/admin_util.py @@ -4,12 +4,12 @@ import time from sqlalchemy import false, func from galaxy import util, web +from galaxy.security.validate_user_input import validate_password from galaxy.util import inflector from galaxy.util.hash_util import new_secure_hash from galaxy.web.form_builder import CheckboxField from tool_shed.util.web_util import escape - log = logging.getLogger(__name__) compliance_log = logging.getLogger('COMPLIANCE') @@ -685,13 +685,8 @@ class Admin(object): user = get_user(trans, user_id) password = kwd.get('password', None) confirm = kwd.get('confirm', None) - if len(password) < 6: - message = "Use a password of at least 6 characters." - status = 'error' - break - elif password != confirm: - message = "Passwords do not match." - status = 'error' + message = validate_password(trans, password, confirm) + if message: break else: user.set_password_cleartext(password) diff --git a/lib/tool_shed/webapp/model/__init__.py b/lib/tool_shed/webapp/model/__init__.py index 78461817067..2e76778e2ba 100644 --- a/lib/tool_shed/webapp/model/__init__.py +++ b/lib/tool_shed/webapp/model/__init__.py @@ -8,6 +8,7 @@ from datetime import ( import tool_shed.repository_types.util as rt_util from galaxy import util from galaxy.model.orm.now import now +from galaxy.security.validate_user_input import validate_password_str from galaxy.util import unique_id from galaxy.util.bunch import Bunch from galaxy.util.dictifiable import Dictifiable @@ -63,6 +64,9 @@ class User(Dictifiable): total_disk_usage = property(get_disk_usage, set_disk_usage) def set_password_cleartext(self, cleartext): + message = validate_password_str(cleartext, cleartext) + if message: + raise Exception("Invalid password: %s" % message) """Set 'self.password' to the digest of 'cleartext'.""" self.password = new_secure_hash(text_type=cleartext) diff --git a/scripts/tool_shed/bootstrap_tool_shed/create_user_with_api_key.py b/scripts/tool_shed/bootstrap_tool_shed/create_user_with_api_key.py index f8facd69e6f..8cae502273b 100644 --- a/scripts/tool_shed/bootstrap_tool_shed/create_user_with_api_key.py +++ b/scripts/tool_shed/bootstrap_tool_shed/create_user_with_api_key.py @@ -4,7 +4,6 @@ from __future__ import print_function import logging import optparse import os -import re import sys from six.moves.configparser import ConfigParser @@ -14,16 +13,17 @@ sys.path.insert(1, os.path.join(os.path.dirname(__file__))) import tool_shed.webapp.config as tool_shed_config from galaxy.security.idencoding import IdEncodingHelper +from galaxy.security.validate_user_input import ( + validate_email_str, + validate_password_str, + validate_publicname_str +) from tool_shed.webapp.model import mapping from bootstrap_util import admin_user_info # noqa: I100,I201 log = logging.getLogger(__name__) -VALID_PUBLICNAME_RE = re.compile(r"^[a-z0-9\-]+$") -VALID_EMAIL_RE = re.compile(r"[^@]+@[^@]+\.[^@]+") - - class BootstrapApplication(object): """ Creates a basic Tool Shed application in order to discover the database connection and use SQL @@ -84,44 +84,15 @@ def create_user(app): def validate(email, password, username): - message = validate_email(email) - if not message: - message = validate_password(password) - if not message: - message = validate_publicname(username) + message = "\n".join([validate_email_str(email), + validate_password_str(password, password), + validate_publicname_str(username)]).rstrip() return message -def validate_email(email): - """Validates the email format.""" - message = '' - if not(VALID_EMAIL_RE.match(email)): - message = "Please enter a real email address." - elif len(email) > 255: - message = "Email address exceeds maximum allowable length." - return message - - -def validate_password(password): - if len(password) < 6: - return "Use a password of at least 6 characters" - return '' - - -def validate_publicname(username): - """Validates the public username.""" - if len(username) < 3: - return "Public name must be at least 3 characters in length" - if len(username) > 255: - return "Public name cannot be more than 255 characters in length" - if not(VALID_PUBLICNAME_RE.match(username)): - return "Public name must contain only lower-case letters, numbers and '-'" - return '' - - if __name__ == "__main__": parser = optparse.OptionParser(description='Create a user with API key.') - parser.add_option('-c', dest='config', action='store', help='.ini file to retried toolshed configuration from') + parser.add_option('-c', dest='config', action='store', help='.ini file to retrieve toolshed configuration from') (args, options) = parser.parse_args() ini_file = args.config config_parser = ConfigParser({'here': os.getcwd()}) From af7226cb52752ac85190d63d0132d7a3e746e933 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Mon, 25 May 2020 00:42:01 +0530 Subject: [PATCH 4/4] Add test to validate password length policy --- test/unit/test_auth.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/unit/test_auth.py b/test/unit/test_auth.py index 557de59cb13..c9b20bc9a7b 100644 --- a/test/unit/test_auth.py +++ b/test/unit/test_auth.py @@ -10,9 +10,16 @@ def test_alwaysreject(): def test_localdb(): user = User(email='testmail', username='tester') - user.set_password_cleartext('test') + user.set_password_cleartext('test123') t = LocalDB() reject = t.authenticate_user(user, 'wrong', {'redact_username_in_logs': False}) - accept = t.authenticate_user(user, 'test', {'redact_username_in_logs': False}) + accept = t.authenticate_user(user, 'test123', {'redact_username_in_logs': False}) assert reject is False assert accept is True + # Password must conform to policy (length etc) + try: + user.set_password_cleartext('test') + except Exception: + pass + else: + raise Exception("Password policy validation failed")