Commits

hasi...@7322e99d-02ea-0310-aa39-e9a107903beb  committed d5c9cd7

AccountManagerPlugin: Continue rewriting `_create_user()` checks, refs #874.

Now moving checks for email address and password input as well.

New unit tests helped me to recognize an unreported bug in the test about
effective configuration of email address verification.
Because `if_enabled` always returns something for a class, the email address
checking was unconditionally mandatory regarding EmailVerificationModule,
even if the module had been disabled. New check uses `is_enabled` instead.

  • Participants
  • Parent commits 1ea63a0

Comments (0)

Files changed (3)

File accountmanagerplugin/trunk/acct_mgr/register.py

 from acct_mgr.api import AccountManager, CommonTemplateProvider, \
                          IAccountRegistrationInspector, _, N_, dgettext, tag_
 from acct_mgr.model import email_associated, set_user_attribute
-from acct_mgr.util import containsAny, if_enabled, is_enabled
+from acct_mgr.util import containsAny, is_enabled
 
 
 def _create_user(req, env, check_permissions=True):
     username = acctmgr.handle_username_casing(req.args.get('username').strip())
     name = req.args.get('name').strip()
     email = req.args.get('email').strip()
-    account = {
-        'username': username,
-        'name': name,
-        'email': email,
-    }
-    error = TracError('')
-    error.account = account
-
-    # Check whether there is also a user or a group with that name.
-    if check_permissions:
-        # NOTE: We can't use 'get_user_permissions(username)' here
-        #   as this always returns a list - even if the user doesn't exist.
-        #   In this case the permissions of "anonymous" are returned.
-        #
-        #   Also note that we can't simply compare the result of
-        #   'get_user_permissions(username)' to some known set of permission,
-        #   i.e. "get_user_permissions('authenticated') as this is always
-        #   false when 'username' is the name of an existing permission group.
-        #
-        #   And again obfuscate whether an existing user or group name
-        #   was responsible for rejection of this username.
-        for (perm_user, perm_action) in \
-                perm.PermissionSystem(env).get_all_permissions():
-            if perm_user.lower() == username.lower():
-                error.message = Markup(_("""
-                    Another account or group already exists, who's name
-                    differs from %s only by case or is identical.
-                    """) % tag.b(username))
-                raise error
-
-    # Validation of username passed.
-
-    password = req.args.get('password')
-    if not password:
-        error.message = _("Password cannot be empty.")
-        raise error
-
-    if password != req.args.get('password_confirm'):
-        error.message = _("The passwords must match.")
-        raise error
-
-    # Validation of password passed.
-
-    if if_enabled(EmailVerificationModule) and acctmgr.verify_email:
-        if not email:
-            error.message = _("You must specify a valid email address.")
-            raise error
-        elif not re.match('^[A-Z0-9._%+-]+@(?:[A-Z0-9-]+\.)+[A-Z]{2,6}$',
-                          email, re.IGNORECASE):
-            error.message = _("""The email address specified appears to be
-                              invalid. Please specify a valid email address.
-                              """)
-            raise error
-        elif email_associated(env, email):
-            error.message = _("""The email address specified is already in
-                              use. Please specify a different one.
-                              """)
-            raise error
-
-    # Validation of email address passed.
-
-    acctmgr.set_password(username, password)
+    acctmgr.set_password(username, req.args.get('password'))
 
     # INSERT new sid, needed as foreign key in some db schemata later on,
     # at least for PostgreSQL.
             module=self.__class__.__name__)
 
 
-class UsernameBasicCheck(GenericRegistrationInspector):
-    """A collection of basic username checks.
+class BasicCheck(GenericRegistrationInspector):
+    """A collection of basic checks.
 
     This includes checking for
-     * emptiness (no user input for username)
-     * some blacklisted characters
+     * emptiness (no user input for username and/or password)
+     * some blacklisted username characters
      * some reserved usernames
-     * a duplicate in configured password stores
+     * a username duplicate in configured password stores
     """
 
     def validate_registration(self, req):
                     differs from %s only by case or is identical.
                     """) % tag.b(username)))
 
+        # Password consistency checks follow.
+        password = req.args.get('password')
+        if not password:
+            raise RegistrationError(_("Password cannot be empty."))
+        elif password != req.args.get('password_confirm'):
+            raise RegistrationError(_("The passwords must match."))
+
+
+class EmailCheck(GenericRegistrationInspector):
+    """A collection of checks for email addresses.
+
+    This check is bypassed, if account verification is disabled.
+    """
+
+    def render_registration_fields(self, req):
+        """Add an email address text input field to the registration form."""
+        # Preserve last input for editing on failure instead of typing
+        # everything again.
+        old_value = req.args.get('email', '').strip()
+        insert = tag.label("Email:", tag.input(type='text', name='email',
+                                               class_='textwidget', size=20,
+                                               value=old_value))
+        # Deferred import required to aviod circular import dependencies.
+        from acct_mgr.web_ui import AccountModule
+        reset_password = AccountModule(self.env).reset_password_enabled
+        verify_account = is_enabled(self.env, EmailVerificationModule) and \
+                         AccountManager(self.env).verify_email
+        if verify_account:
+            # TRANSLATOR: Registration form hints for a mandatory input field.
+            hint = tag.p(_("""The email address is required for Trac to send
+                           you a verification token."""), class_='hint')
+            if reset_password:
+                hint = tag(hint, tag.p(_("""
+                           Entering your email address will also enable you
+                           to reset your password if you ever forget it.
+                           """), class_='hint'))
+            return tag(insert, hint), {}
+        elif reset_password:
+            # TRANSLATOR: Registration form hint, if email input is optional.
+            hint = tag.p(_("""Entering your email address will enable you to
+                           reset your password if you ever forget it. """),
+                         class_='hint')
+            return dict(optional=tag(insert, hint)), {}
+        else:
+            # Always return the email text input itself as optional field.
+            return dict(optional=insert), {}
+
+    def validate_registration(self, req):
+        acctmgr = AccountManager(self.env)
+        email = req.args.get('email').strip()
+
+        if is_enabled(self.env, EmailVerificationModule) and \
+                acctmgr.verify_email:
+            if not email:
+                raise RegistrationError(_(
+                    "You must specify a valid email address."))
+            elif not re.match('^[A-Z0-9._%+-]+@(?:[A-Z0-9-]+\.)+[A-Z]{2,6}$',
+                              email, re.IGNORECASE):
+                raise RegistrationError(_("""
+                    The email address specified appears to be invalid.
+                    Please specify a valid email address.
+                    """))
+            elif email_associated(self.env, email):
+                raise RegistrationError(_("""
+                    The email address specified is already in use.
+                    Please specify a different one.
+                    """))
+
 
 class UsernamePermCheck(GenericRegistrationInspector):
     """Check for usernames referenced in the permission system.
 
     _register_check = OrderedExtensionsOption(
         'account-manager', 'register_check', IAccountRegistrationInspector,
-        default='UsernameBasicCheck, UsernamePermCheck',
+        default='BasicCheck, EmailCheck, UsernamePermCheck',
         include_missing=False,
         doc="""Ordered list of IAccountRegistrationInspector's to use for
         registration checks.""")
             req.redirect(req.href.prefs('account'))
         action = req.args.get('action')
         data = {
-            '_dgettext': dgettext,
-            'acctmgr': {
-                'username': None,
-                'name': None,
-                'email': None,
-            },
-            'ignore_auth_case': self.config.getbool('trac', 'ignore_auth_case')
+                '_dgettext': dgettext,
+                  'acctmgr': dict(username=None, name=None),
+         'ignore_auth_case': self.config.getbool('trac', 'ignore_auth_case')
         }
         verify_enabled = is_enabled(self.env, EmailVerificationModule) and \
                          self.acctmgr.verify_email
                 _create_user(req, self.env)
             except RegistrationError, e:
                 chrome.add_warning(req, e.message)
-            except TracError, e:
-                data['registration_error'] = e.message
-                data['acctmgr'].update(getattr(e, 'account', ''))
+                account = {
+                    'username': self.acctmgr.handle_username_casing(
+                                    req.args.get('username').strip()),
+                        'name': req.args.get('name').strip(),
+                }
+                data['acctmgr'].update(account)
             else:
                 if verify_enabled:
                     chrome.add_notice(req, Markup(tag.span(Markup(_(
         # Collect additional fields from IAccountRegistrationInspector's.
         fragments = dict(required=[], optional=[])
         for inspector in self._register_check:
-            fragment, f_data = inspector.render_registration_fields(req)
+            try:
+                fragment, f_data = inspector.render_registration_fields(req)
+            except TypeError, e:
+                # Add some robustness by logging the most likely errors.
+                self.env.log.warn("%s.render_registration_fields failed: %s"
+                                  % (inspector.__class__.__name__, e))
+                fragment = None
             if fragment:
                 try:
                     if 'optional' in fragment.keys():
                     data.update(f_data)
         data['required_fields'] = fragments['required']
         data['optional_fields'] = fragments['optional']
-        # Deferred import required to aviod circular import dependencies.
-        from acct_mgr.web_ui import AccountModule
-        data['reset_password_enabled'] = AccountModule(self.env
-                                                      ).reset_password_enabled
         return 'register.html', data, None
 
 

File accountmanagerplugin/trunk/acct_mgr/templates/register.html

                      class="textwidget" size="20" />
             </label>
           </div>
-          <div py:if="verify_account_enabled">
-            <label>Email:
-              <input type="text" name="email" class="textwidget" size="20"
-                value="${acctmgr.email}" />
-            </label>
-            <p class="hint">The email address is required for Trac to send
-              you a verification token.
-            </p>
-            <p class="hint" py:if="reset_password_enabled">
-              Entering your email address will also enable you to reset your
-              password if you ever forget it.
-            </p>
-          </div>
           <div py:for="field in required_fields">
             <!--! Additional required fields can be included below. -->
             ${field}
                 value="${acctmgr.name}"/>
             </label>
           </div>
-          <div py:if="not verify_account_enabled">
-            <label>Email:
-              <input type="text" name="email" class="textwidget" size="20"
-                value="${acctmgr.email}"/>
-            </label>
-            <p class="hint" py:if="reset_password_enabled">
-              Entering your email address will enable you to reset your
-              password if you ever forget it.
-            </p>
-          </div>
           <div py:for="field in optional_fields">
             <!--! Additional optional fields can be included below. -->
             ${field}

File accountmanagerplugin/trunk/acct_mgr/tests/register.py

 from acct_mgr.admin  import AccountManagerAdminPanels
 from acct_mgr.api  import AccountManager, IAccountRegistrationInspector
 from acct_mgr.db  import SessionStore
-from acct_mgr.register  import GenericRegistrationInspector, \
+from acct_mgr.model  import set_user_attribute
+from acct_mgr.register  import BasicCheck, EmailCheck, \
+                               GenericRegistrationInspector, \
                                RegistrationError, RegistrationModule, \
-                               UsernameBasicCheck, UsernamePermCheck
+                               UsernamePermCheck
 
 
 class _BaseTestCase(unittest.TestCase):
     def setUp(self):
         self.env = EnvironmentStub(
-                enable=['trac.*', 'acct_mgr.*'])
+                enable=['trac.*', 'acct_mgr.admin.*'])
         self.env.path = tempfile.mkdtemp()
         self.perm = PermissionSystem(self.env)
 
             self.assertEqual(e.title, 'Registration Error')
 
 
-class UsernameBasicCheckTestCase(_BaseTestCase):
+class BasicCheckTestCase(_BaseTestCase):
     def setUp(self):
         _BaseTestCase.setUp(self)
+        self.env = EnvironmentStub(
+                enable=['trac.*', 'acct_mgr.admin.*',
+                        'acct_mgr.pwhash.HtDigestHashMethod'])
+        self.env.path = tempfile.mkdtemp()
         self.env.config.set('account-manager', 'password_store',
                             'SessionStore')
         store = SessionStore(self.env)
         store.set_password('registered_user', 'password')
 
     def test_check(self):
-        check = UsernameBasicCheck(self.env)
+        check = BasicCheck(self.env)
         req = self.req
         # Inspector doesn't provide additional fields.
         field_res = check.render_registration_fields(req)
         req.args['username'] = 'registered_user'
         self.assertRaises(RegistrationError, check.validate_registration,
                           req)
+        # 5th attempt: Valid username, but no password.
+        req.args['username'] = 'user'
+        self.assertRaises(RegistrationError, check.validate_registration,
+                          req)
+        # 6th attempt: Valid username, no matching passwords.
+        req.args['password'] = 'password'
+        self.assertRaises(RegistrationError, check.validate_registration,
+                          req)
+        # 7th attempt: Finally some valid input.
+        req.args['password_confirm'] = 'password'
+        self.assertEqual(check.validate_registration(req), None)
+
+
+class EmailCheckTestCase(_BaseTestCase):
+    """Needs several test methods, because dis-/enabling a component doesn't
+    work within one method.
+    """
+
+    def test_verify_mod_disabled(self):
+        """Registration challenges with EmailVerificationModule disabled."""
+        self.env = EnvironmentStub(
+                enable=['trac.*', 'acct_mgr.admin.*'])
+        self.env.path = tempfile.mkdtemp()
+
+        check = EmailCheck(self.env)
+        req = self.req
+
+        self.env.config.set('account-manager', 'verify_email', False)
+        self.assertEqual(check.validate_registration(req), None)
+        # Check should be skipped regardless of AccountManager settings.
+        self.env.config.set('account-manager', 'verify_email', True)
+        self.assertEqual(check.validate_registration(req), None)
+
+    def test_verify_conf_changes(self):
+        """Registration challenges with EmailVerificationModule enabled."""
+        self.env = EnvironmentStub(
+                enable=['trac.*', 'acct_mgr.admin.*', 'acct_mgr.register.*'])
+        self.env.path = tempfile.mkdtemp()
+        set_user_attribute(self.env, 'admin', 'email', 'admin@foo.bar')
+
+        check = EmailCheck(self.env)
+        req = self.req
+
+        # Inspector provides the email text input field.
+        old_email_input = 'email@foo.bar'
+        req.args.update(dict(email=old_email_input))
+        field_res = check.render_registration_fields(req)
+        self.assertEqual(len(field_res), 2)
+        self.assertTrue(Markup(field_res[0]).startswith('<label>Email:'))
+        # Make sure, that old input is preserved on failure.
+        self.assertTrue(old_email_input in Markup(field_res[0]))
+        self.assertEqual(field_res[1], {})
+        req.args.update(dict(email=''))
+
+        # 1st: Initially try with account verification disabled by setting.
+        self.env.config.set('account-manager', 'verify_email', False)
+        self.assertEqual(check.validate_registration(req), None)
+        # 2nd: Again no email, but now with account verification enabled. 
+        self.env.config.set('account-manager', 'verify_email', True)
+        self.assertRaises(RegistrationError, check.validate_registration,
+                          req)
+        # 3rd attempt: Malformed email address.
+        req.args['email'] = 'email'
+        self.assertRaises(RegistrationError, check.validate_registration,
+                          req)
+        # 4th attempt: Valid email, but already registered with a username.
+        req.args['email'] = 'admin@foo.bar'
+        self.assertRaises(RegistrationError, check.validate_registration,
+                          req)
         # 5th attempt: Finally some valid input.
-        req.args['username'] = 'user'
+        req.args['email'] = 'email@foo.bar'
         self.assertEqual(check.validate_registration(req), None)
 
 
         self.req.args['username'] = 'admin'
         self.req.args['email'] = 'admin@foo.bar'
         self.env.config.set('account-manager', 'register_check',
-                            'UsernameBasicCheck')
+                            'BasicCheck')
         response = self.rmod.process_request(self.req)
         self.assertEqual(response[0], self.reg_template)
         # Custom configuration: No check at all, if you insist.
 def suite():
     suite = unittest.TestSuite()
     suite.addTest(unittest.makeSuite(DummyRegInspectorTestCase, 'test'))
-    suite.addTest(unittest.makeSuite(UsernameBasicCheckTestCase, 'test'))
+    suite.addTest(unittest.makeSuite(BasicCheckTestCase, 'test'))
+    suite.addTest(unittest.makeSuite(EmailCheckTestCase, 'test'))
     suite.addTest(unittest.makeSuite(UsernamePermCheckTestCase, 'test'))
     suite.addTest(unittest.makeSuite(RegistrationModuleTestCase, 'test'))
     return suite