Rework password checks in account renewal to rely on check_hash / check_scramble

calls in the recently introduced early_validation_checks during account request
submission where original password is still available.

Fix a use of load_account_dict using configuration where logger is expected.

Add unit tests to cover a number of early_validation_checks cases. Still more
tests pending e.g. to test renewal with legal and illegal password change.

Author: jonasbardino

mig/shared/accountreq.py

@@ -51,7 +51,7 @@
     gdp_distinguished_field
 from mig.shared.fileio import delete_file, make_temp_file
 from mig.shared.notification import notify_user
-from mig.shared.pwcrypto import check_hash
+from mig.shared.pwcrypto import check_hash, check_scramble
 # Expose some helper variables for functionality backends
 from mig.shared.safeinput import name_extras, password_extras, \
     password_min_len, password_max_len, valid_password_chars, \
@@ -1231,24 +1231,31 @@ def early_validation_checks(configuration, raw_request, service, username,
 
     client_id = get_user_id(configuration, raw_request)
     db_path = default_db_path(configuration)
-    user_dict = load_user_dict(configuration, client_id, db_path)
+    user_dict = load_user_dict(logger, client_id, db_path)
     if user_dict:
-        # Renewal or password change
+        # Renewal or password change - check against existing pw and status
+        scrambled = user_dict.get('password', None)
+        hashed = user_dict.get('password_hash', None)
+        account_status = user_dict.get('status', 'active')
         authorized = raw_request.get('authorized', None)
         reset_token = raw_request.get('reset_token', None)
-        account_status = raw_request.get('status', 'active')
-        if not authorized and not reset_token:
-            hashed = user_dict.get('password_hash', None)
-            if not check_hash(configuration, service, username, password,
-                              hashed):
-                logger.warning('illegal password change in request from %r' %
-                               client_id)
-                raw_request['invalid'].append(illegal_pw_change)
-        elif account_status not in ('temporal', 'active', 'inactive'):
+        if hashed:
+            raw_request['pw_changed'] = check_hash(configuration, service,
+                                                   username, password, hashed)
+        elif scrambled:
+            raw_request['pw_changed'] = check_scramble(
+                configuration, service, username, password, scrambled,
+                salt=configuration.site_password_salt)
+
+        if account_status not in ('temporal', 'active', 'inactive'):
             logger.warning('existing account for %r is %s and not renewable'
                            % (client_id, account_status))
             raw_request['invalid'].append(renewal_blocked)
-        else:
+        if raw_request['pw_changed'] and not authorized and not reset_token:
+            logger.warning('illegal password change in request from %r' %
+                           client_id)
+            raw_request['invalid'].append(illegal_pw_change)
+        if not raw_request.get('invalid', []):
             logger.debug('account renewal from %r looks alright' % client_id)
     elif existing_user_collision(configuration, raw_request, client_id):
         # TODO: drop and rely solely on live check in janitor to avoid races?

mig/shared/useradm.py

@@ -459,10 +459,10 @@ def verify_user_peers(configuration, db_path, client_id, user, now, verify_peer,
 
 
 def create_user_in_db(configuration, db_path, client_id, user, now, authorized,
-                      reset_token, reset_auth_type, accepted_peer_list, force,
-                      verbose, ask_renew, default_renew, do_lock,
-                      from_edit_user, ask_change_pw, auto_create_db,
-                      create_backup):
+                      reset_token, reset_auth_type, pw_changed,
+                      accepted_peer_list, force, verbose, ask_renew,
+                      default_renew, do_lock, from_edit_user, ask_change_pw,
+                      auto_create_db, create_backup):
     """Handle all the parts of user creation or renewal relating to the user
     datatbase.
     """
@@ -569,14 +569,14 @@ def create_user_in_db(configuration, db_path, client_id, user, now, authorized,
             # password alone.
             # External OpenID users do not provide a password so again any
             # existing password should be left alone on renewal.
-            # The password_hash field is not guaranteed to exist.
+            # The password_hash field is not guaranteed to exist and it varies
+            # depending on the current random seed and assigned salt. So we use
+            # check_password during submit and save result in pw_changed.
             if not user['password']:
                 user['password'] = user['old_password']
             if not user.get('password_hash', ''):
                 user['password_hash'] = user['old_password_hash']
-            password_changed = (user['old_password'] != user['password'] or
-                                user['old_password_hash'] != user['password_hash'])
-            if password_changed:
+            if pw_changed:
                 # Allow password change if it's directly authorized with login
                 # or authorized through a simple reset challenge (reset_token).
                 if not authorized and reset_token:
@@ -1062,6 +1062,11 @@ def create_user(user, conf_path, db_path, force=False, verbose=False,
         reset_auth_type = user.get('auth', ['UNKNOWN'])[-1]
         # Always remove any reset_token fields before DB insert
         del user['reset_token']
+    pw_changed = False
+    if 'pw_changed' in user:
+        pw_changed = user['pw_changed']
+        # Always remove any pw_changed fields before DB insert
+        del user['pw_changed']
 
     _logger.info('trying to create or renew user %r' % client_id)
     if verbose:
@@ -1090,9 +1095,10 @@ def create_user(user, conf_path, db_path, force=False, verbose=False,
 
     created = create_user_in_db(configuration, db_path, client_id, user, now,
                                 authorized, reset_token, reset_auth_type,
-                                accepted_peer_list, force, verbose, ask_renew,
-                                default_renew, do_lock, from_edit_user,
-                                ask_change_pw, auto_create_db, create_backup)
+                                pw_changed, accepted_peer_list, force, verbose,
+                                ask_renew, default_renew, do_lock,
+                                from_edit_user, ask_change_pw, auto_create_db,
+                                create_backup)
     # Mark user updated for all logins
     update_account_expire_cache(configuration, created)
     update_account_status_cache(configuration, created)

tests/test_mig_shared_accountreq.py

@@ -204,6 +204,95 @@ def test_peers_prefilter_email_reject(self):
                                                        user)
             self.assertFalse(check)
 
+    def test_early_validation_checks_valid_new_simple(self):
+        service = 'migoid'
+        full_name = 'Valid Test Name'
+        email = 'john@doe.org'
+        dummy_pw = 'PW74deb6609F109f504d'
+        dummy_pw_hash = 'DUMMYPWHASH'
+        user = {'full_name': full_name, 'organization': 'Test Org',
+                'organizational_unit': '', 'email': email,
+                'password_hash': dummy_pw_hash, 'comment': ''}
+        fill_distinguished_name(user)
+        checked = accountreq.early_validation_checks(self.configuration, user,
+                                                     service, email, dummy_pw)
+        print("DEBUG: early checks on valid simple req: %s" % checked)
+        self.assertEqual(checked['invalid'], [], "early validation failed")
+
+    def test_early_validation_checks_valid_new_peers(self):
+        self.configuration.site_enable_peers = True
+        self.configuration.site_peers_explicit_fields = ['full_name', 'email']
+        self.configuration.site_peers_prefilter = [
+            ('peers_email', r'^.+@([a-z0-9]+\.)*ku\.dk$')]
+        service = 'migoid'
+        full_name = 'Valid Test Name'
+        email = 'john@doe.org'
+        peers_full_name = 'Valid Peer Name'
+        dummy_pw = 'PW74deb6609F109f504d'
+        dummy_pw_hash = 'DUMMYPWHASH'
+        accept = ['john.doe@science.ku.dk', 'abc123@ku.dk',
+                  'john.doe@a.b.c.ku.dk']
+        self.configuration.site_peers_prefilter = [
+            ('peers_email', r'^.+@([a-z0-9]+\.)*ku\.dk$')]
+        user = {'full_name': full_name, 'organization': 'Test Org',
+                'organizational_unit': '', 'email': email,
+                'password_hash': dummy_pw_hash, 'comment': '',
+                'peers_full_name': [peers_full_name], 'peers_email': []}
+        fill_distinguished_name(user)
+        for addr in accept:
+            user['peers_email'] = [addr]
+            username = user['email']
+            checked = accountreq.early_validation_checks(self.configuration,
+                                                         user, service,
+                                                         username, dummy_pw)
+            print("DEBUG: early checks on valid peers req: %s" % checked)
+            self.assertEqual(checked['invalid'], [], "early validation failed")
+
+    # TODO: add test valid renew
+
+    def test_early_validation_checks_invalid_new_simple(self):
+        service = 'migoid'
+        full_name = 'InvalidTestName'
+        email = 'john@doe.org'
+        dummy_pw = 'PW74deb6609F109f504d'
+        dummy_pw_hash = 'DUMMYPWHASH'
+        user = {'full_name': full_name, 'organization': 'Test Org',
+                'organizational_unit': '', 'email': email,
+                'password_hash': dummy_pw_hash, 'comment': ''}
+        fill_distinguished_name(user)
+        checked = accountreq.early_validation_checks(self.configuration, user,
+                                                     service, email, dummy_pw)
+        print("DEBUG: early checks on invalid simple req: %s" % checked)
+        self.assertTrue(checked['invalid'], "early validation failed")
+
+    def test_early_validation_checks_invalid_new_peers(self):
+        self.configuration.site_enable_peers = True
+        self.configuration.site_peers_explicit_fields = ['email']
+        self.configuration.site_peers_prefilter = [
+            ('peers_email', r'^.+@([a-z0-9]+\.)*ku\.dk$')]
+        service = 'migoid'
+        full_name = 'Valid Test Name'
+        email = 'john@doe.org'
+        peers_full_name = 'Valid Peer Name'
+        dummy_pw = 'PW74deb6609F109f504d'
+        dummy_pw_hash = 'DUMMYPWHASH'
+        user = {'full_name': full_name, 'organization': 'Test Org',
+                'organizational_unit': '', 'email': email,
+                'password_hash': dummy_pw_hash, 'comment': '',
+                'peers_full_name': [peers_full_name], 'peers_email': []}
+        fill_distinguished_name(user)
+        reject = ['', 'john@doe.org', 'a@b.c.org', 'a@ku.dk.com',
+                  'a@sci.ku.dk.org']
+        for addr in reject:
+            user['peers_email'] = addr
+            username = user['email']
+            checked = accountreq.early_validation_checks(self.configuration, user,
+                                                         service, email, dummy_pw)
+            print("DEBUG: early checks on invalid peers req: %s" % checked)
+            self.assertTrue(checked['invalid'], "early validation failed")
+
+    # TODO: add test invalid renew
+
 
 if __name__ == '__main__':
     testmain()