[patch] Add new fields to the registration screen, and a validation system for new registrations

[Noah Kantrowitz]

Not sure if I like the interface, but it's useful to have. This patch creates an IRegistrationFieldProivder extension point to contribute fields to the registration screen. Currently these need to be fragments or Markup()'d HTML. It also creates IRegistrationManipulator to then do something with these fields, and possibly reject the registration as invalid.

[dr2chase]

This looks like it subsumes something that I need, which is one of those "I agree to the following blah-blah-blah" followed by a yes/no radio button. I've figured out how to do it in the non-general way, but I don't see how I would hook the same thing in to this patch -- that is, I don't know enough about this framework to know where to put my IRegistrationFieldProvider. (Do I need to write a plugin? What's the end-to-end series of steps that turns that plugin into activated code?)

[dr2chase]

Answering my own question, here's a skeleton that is loaded, inserts a silly phrase, and does no accept filtering. This can be dropped in a project's plugins directory.

from acct_mgr.api import IRegistrationFieldProvider, IRegistrationManipulator
from trac.core import *
from trac.config import Option, ExtensionOption

class TermsAndConditions(Component):
  """Inject Terms and Conditions onto registration web page, require
  a positive answer.
  """

  implements(IRegistrationFieldProvider)

# Return an iterable of (fragment, type) where type is either 'required' or 'optional'.
  def get_registration_fields(self, req):
   return [("All your base are belong to US!", 'required')]

class TermsAndConditionsCheck(Component):
  """Validate reply to terms and conditions
  """
  implements(IRegistrationManipulator)

# Should return an iterable of errors (with no items if all is well).
  def validate_registration(self, req, username, password):
    return ()

[dr2chase]

And also; for license acceptance, and that sort of thing, I think it might be better if the iterated required fields preceded the username and password. I know that I can make that change in my own copy, but I suspect that it will be usual-case to want to make that change. The lawyers that I've talked to are happiest if the license is right there in the user's face, so that it literally must be scrolled past to get to the gimme-my-account buttons.

(Needless to say, I am in favor of this patch, or something like it, being rolled into the standard release.)

[dr2chase]

Problems with this patch:

• validate_registration(req, username, password) in api.py
  does not match the call
  errors += list(m.validate_registration(req)) in web_ui.py.
• The insert registration fields are run through a quoting process. This method:

  def get_registration_fields(self, req):
    return [('<b>Foo</b>', 'required')]
  

  produces (in the html source) <b>Foo</b>.

[dr2chase]

Replying to dr2chase:

def get_registration_fields(self, req):

return [('<b>Foo</b>', 'required')]

}}} produces (in the html source) <b>Foo</b>.

and the answer is, notice the remark about Markup in the original:

from trac.util.html import Markup
...
  def get_registration_fields(self, req):
   return [(Markup('<b>Foo</b>'), 'required')]

[Pedro Algarvio, aka, s0undt3ch]

Implemented for trac 0.11 ​here, revisions ​65 and ​66. Custom fields provided can also show up on user preferences.

[Nicholas Bergson-Shilcock]

I wrote a somewhat similar patch, RegistrationConfirmationPatch, before I saw this ticket. The patch provides an extension point that allows plugins to add arbitrary markup to the registration form and also validate the extra input (I also wrote a CAPTCHA plugin, SimpleCaptchaPlugin, that gives a good idea as to how the interface can be used).

It differs from this patch in that it uses a single interface, IRegistrationConfirmation, to provide both form additions and validation, rather than splitting them up into two separate interfaces.

[Thijs Triemstra]

Replying to dr2chase:

And also; for license acceptance, and that sort of thing, I think it might be better if the iterated required fields preceded the username and password. I know that I can make that change in my own copy, but I suspect that it will be usual-case to want to make that change. The lawyers that I've talked to are happiest if the license is right there in the user's face, so that it literally must be scrolled past to get to the gimme-my-account buttons.

Having people accept our (MIT) license before they can register, so that their contributions's copyright is assigned to our project, is a great idea and I would vote for such an enhancement.

[dr2chase]

A patch for a more recent version of account manager, works with 0.11

[dr2chase]

s0undt3ch, I am not seeing your patches. I attached my updated patch above (it's lightly tested with 0.11 and an old terms-of-use+captcha plugin).

I will attempt to get the TOU+CAPTCHA plugin turned into open source code; it's part of an open source project, in theory it should be pretty easy.

[Pedro Algarvio, aka, s0undt3ch]

Replying to dr2chase:

s0undt3ch, I am not seeing your patches. I attached my updated patch above (it's lightly tested with 0.11 and an old terms-of-use+captcha plugin).

I will attempt to get the TOU+CAPTCHA plugin turned into open source code; it's part of an open source project, in theory it should be pretty easy.

Website should be up now.

[Steffen Hoffmann]

Replying to nicholasbs:

I wrote a somewhat similar patch, RegistrationConfirmationPatch, before I saw this ticket. ![...]

Would you like to help me with integration, as captcha support is on my ToDo list as well?

[James Teh]

I'd be interested in helping to get this integrated.

It could be argued that the approach taken by RegistrationConfirmationPatch is more generic in that it allows arbitrary markup to be added, rather than restricting it to a concept of fields. That said, the markup can only be put in one place with RegistrationConfirmationPatch, so perhaps having optional and required fields is better because it allows you to specify on which part of the page the markup should be placed. Still, it might be simpler to just have a {page_section: fragment} dict, rather than an iterable of fields.

The other question is whether or not to split the pre and post stages into two interfaces as in the patch for this ticket. One interface seems simpler to me, though that does make it less clean when you just want to implement the post (verification) stage. (If you just want to change the form, you can just use a site template.)

Thoughts?

[Steffen Hoffmann]

Replying to jteh:

I'd be interested in helping to get this integrated.

Great! Let's do it then.

[...] Thoughts?

One thing not mentioned here is one of my all-time-favorites: i18n. Any change has to include (preparation for) full i18n support to be acceptable.

Anyway considerable preparation has already been done by patches here as well as the RegistrationConfirmationPatch, so it seems at least doable within hours, not days. Your assistance for testing added I'm willing to solve this issue.

I suggest to drop the simpler multiple-interface-approach. This was my initial feeling and it has been confirmed by nicholasbs's design. Let's take the interface suggested by RegistrationConfirmationPatch as as starting point, maybe adapt as needed to allow great flexibility for re-use with a variety of (existing and future) pre-registration plugins (for AcctMng).

My goal is to not only make RegistrationConfirmationPatch obsolete in the end of the process but at least to provide an upgrade path for early adopters of it's interface, if we'll end up with a changed and backwards-incompatible on. And, if you, me or anyone else joining in too feels up to the task, actually provide patches for such existing code, or try to integrate it, if it as long pending issues like the RegistrationConfirmationPatch.

If we can mark one or more projects as obsolete by integrating idea and/or code into AcctMgr, this would be a most helpful outcome. Thanks in advance for your attention.

Last thought for now: I operate and develop in Trac0.13dev with Python2.6 on Debian GNU/Linux 6.0 (Squeeze, stable). I suggest to aim at compatibility with Trac0.11 at most, if you'll be able to test this. I've been unable until now (restricted by time) to setup a 0.11 test environment that would certainly require an older Python version (2.4?) as well to be relevant. What setup(s) could you provide for code validation?

[James Teh]

Replying to hasienda:

One thing not mentioned here is one of my all-time-favorites: i18n. Any change has to include (preparation for) full i18n support to be acceptable.

Sounds good. Are there any i18n concerns for the core functionality? I'm not familiar with i18n in Trac 0.12 yet, but as I see it, the core functionality only takes user visible strings from plugins, so the plugins will have to provide the i18n support.

Your assistance for testing added I'm willing to solve this issue.

I can try to help with the code as well if needed, though I'll be unable to do much for the next week or so.

I suggest to drop the simpler multiple-interface-approach.

Sounds good.

Let's take the interface suggested by RegistrationConfirmationPatch as as starting point,

There are two issues with this interface:

• The content added to the form should probably be a genshi fragment; i.e. not explicitly markup.
• As I noted earlier, we may need a way to add content to different sections of the registration form. I suggested a dict of {"required": fragment, "optional": fragment}. Maybe this isn't necessary.

Last thought for now: I operate and develop in Trac0.13dev with Python2.6 on Debian GNU/Linux 6.0 (Squeeze, stable). I suggest to aim at compatibility with Trac0.11 at most, if you'll be able to test this.

The best i can do is Trac 0.12 (stable) on Python 2.6, I'm afraid.

[Steffen Hoffmann]

Replying to jteh:

Sounds good. Are there any i18n concerns for the core functionality? I'm not familiar with i18n in Trac 0.12 yet, but as I see it, the core functionality only takes user visible strings from plugins, so the plugins will have to provide the i18n support.

True. I'll just make sure that we'll add only properly i18n-escaped messages. The rest can be handled by translators.

I can try to help with the code as well if needed, though I'll be unable to do much for the next week or so.

Ok, this is not my no. 1 priority too. Let's see, how fast we can do. At least we'll not completely forget about it, as long as one of us is still aware of it.

Let's take the interface suggested by RegistrationConfirmationPatch as as starting point,

There are two issues with this interface:

• The content added to the form should probably be a genshi fragment; i.e. not explicitly markup.
• As I noted earlier, we may need a way to add content to different sections of the registration form. I suggested a dict of {"required": fragment, "optional": fragment}. Maybe this isn't necessary.

I've seen that. I guess we can achieve considerable flexibility by a couple of placeholders, but keep it's number low and ideally make self-explaining names.

The best i can do is Trac 0.12 (stable) on Python 2.6, I'm afraid.

Uh, so I'll have to ask other fellow devs or finally to the setup on my own. But this shouldn't stop us now.

[Steffen Hoffmann]

Still like the conclusions we've come to, but let's postpone this to the next release cycle, ok?

[Steffen Hoffmann]

(In [11916]) AccountManagerPlugin: Separate code for account registration, refs #874.

[Steffen Hoffmann]

(In [11917]) AccountManagerPlugin: Add interface for configurable user registration procedure, refs #874, #2707, #2897, #4651, #5295, #7577, #8076.

The current user registration process lacks flexibility, as can be witnessed by the history of one of the oldest still pending tickets for this plugin: #874.

[Steffen Hoffmann]

(In [11920]) AccountManagerPlugin: Fix some imports, that I missed in [11916], refs #874.

[Steffen Hoffmann]

(In [11921]) AccountManagerPlugin: Transform function _create_user() to new-style check, refs #874.

Let's start with username checks, and add simple unit tests for them too.

Beware: Registration checks for the admin panel will be rewritten a bit later.

[Steffen Hoffmann]

(In [11922]) 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.

[Steffen Hoffmann]

(In [11923]) AccountManagerPlugin: Finish registration check rewrite, refs #874, #2707, #2897, #4651, #5295, #7577, #8076.

Now moving check execution into AccountManager, and _create_user() is gone, replaced by more clearly structured and modularized code.

Registration checking for requests from the admin panel is re-activated and minor improvements related to RegistrationError processing done too.

Note: The test, that detected admin requests to skip additional checking of existing permissions for each new username, has been done differently. In the future each check has to decide on its own, like this is done in the UsernamePermCheck now.

[Steffen Hoffmann]

(In [11924]) AccountManagerPlugin: Early alteration of IAccountRegistrationInspector, refs #874.

Better pass the data between implementations of the interface directly.

[Steffen Hoffmann]

(In [11982]) AccountManagerPlugin: Fix 'UPDATE_INTERVAL' is not defined, refs #874.

This change restores a required value, that got accidently deleted in [11916].

[Steffen Hoffmann]

(In [12028]) AccountManagerPlugin: Restore translatable email input field label, refs #874.

And I recover pre-existing translations (from before [11930]) here too.

[Steffen Hoffmann]

(In [12046]) AccountManagerPlugin: Restore translatable email input field label, refs #874.

And I recover pre-existing translations (from before [11930]) here too.

[Steffen Hoffmann]

(In [12048]) AccountManagerPlugin: Add some simple registration checks for fighting bots, refs #874 and #7577.

There is a mandatory field with required input provided as hint below. The field is only visible and the check working after you've assigned a value to the new option register_basic_token.

"Plan B" in the list of suggested improvements for new user registration is a hidden input field, that might get only ever filled by bot clients. Unlike the other one above this check always works - without configuration.

[Steffen Hoffmann]

(In [12398]) AccountManagerPlugin: Releasing version 0.4, pushing development to acct_mgr-0.5dev.

Availability of that code as stable release closes #874, #3459, #4677, #5295, #5691, #6616, #7577, #8076, #8685, #8770, #8791, #8990, #9052, #9079, #9090, #9139, #9246, #9252, #9547, #9618, #9676, #9843, #9852, #9940, #10023, #10028, #10123, #10142, #10204, #10276, #10397, #10412, #10594, #10625 and #10644.

Some more issues have been worked-on, yet without confirmed resolution, refs #5464 (for JiraToTracIntegration), #8927 and #10134.

And finally there are some issues and enhancement requests showing progress, but known to require more work to resolve them satisfactorily, refs #843, #1600, #5964, #8217, #8933.

Thanks to all contributors and followers, that enabled and encouraged a good portion of this development work.