Modify

Opened 8 years ago

Closed 21 months ago

#874 closed enhancement (fixed)

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

Reported by: coderanger Owned by: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: register SPAM prevention
Cc: thijs, rjollos Trac Release: 0.11

Description

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.

Attachments (2)

acct_mgr-validation.patch (3.3 KB) - added by coderanger 8 years ago.
acct_mgr_0.11.patch (3.8 KB) - added by dr2chase 5 years ago.
A patch for a more recent version of account manager, works with 0.11

Download all attachments as: .zip

Change History (30)

Changed 8 years ago by coderanger

comment:1 Changed 7 years ago by 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?)

comment:2 Changed 7 years ago by 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 ()

comment:3 follow-up: Changed 7 years ago by 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.)

comment:4 follow-up: Changed 7 years ago by 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) &lt;b&gt;Foo&lt;/b&gt;.

comment:5 in reply to: ↑ 4 Changed 7 years ago by dr2chase

Replying to dr2chase:

def get_registration_fields(self, req):

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

}}}
produces (in the html source) &lt;b&gt;Foo&lt;/b&gt;.

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')]

comment:6 Changed 6 years ago by s0undt3ch

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

comment:7 follow-up: Changed 6 years ago by nicholasbs

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.

comment:8 in reply to: ↑ 3 Changed 5 years ago by thijs

  • Cc thijs added

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.

Changed 5 years ago by dr2chase

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

comment:9 follow-up: Changed 5 years ago by 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.

comment:10 in reply to: ↑ 9 Changed 5 years ago by 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.

comment:11 in reply to: ↑ 7 Changed 4 years ago by hasienda

  • Keywords register SPAM prevention added
  • Owner changed from mgood to hasienda
  • Summary changed from [PATCH] Add new fields to the registration screen, and a validation system for new registrations to [patch] Add new fields to the registration screen, and a validation system for new registrations

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?

comment:12 follow-up: Changed 3 years ago by jteh

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?

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by hasienda

  • Cc rjollos added
  • Status changed from new to assigned
  • Trac Release changed from 0.10 to 0.11

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?

comment:14 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by jteh

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.

comment:15 in reply to: ↑ 14 Changed 3 years ago by hasienda

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.

comment:16 Changed 3 years ago by hasienda

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

comment:17 Changed 2 years ago by hasienda

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

comment:18 Changed 2 years ago by hasienda

(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.

comment:19 Changed 2 years ago by hasienda

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

comment:20 Changed 2 years ago by hasienda

(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.

comment:21 Changed 2 years ago by hasienda

(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.

comment:22 Changed 2 years ago by hasienda

(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.

comment:23 Changed 2 years ago by hasienda

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

Better pass the data between implementations of the interface directly.

comment:24 Changed 2 years ago by hasienda

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

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

comment:25 Changed 2 years ago by hasienda

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

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

comment:26 Changed 2 years ago by hasienda

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

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

comment:27 Changed 2 years ago by hasienda

(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.

comment:28 Changed 21 months ago by hasienda

  • Resolution set to fixed
  • Status changed from assigned to closed

(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.

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.