Commas in username causes error in nosy list

[ericvsmith]

User "Oliver Too, Eh?" created https://bugs.python.org/issue36727.

Because of the comma in the username, I couldn't reply to the issue via the bpo UI. It gave me an error: "property nosy: 'Oliver Too' is not a user.", then reformatted the nosy list as "Eh?,Oliver Too,docs@python,eric.smith,xtreak". I tried various combination of quotes and escape backslashes, to no avail. I ended up dropping the user from the nosy list and notifying them via direct email.

@tirkarthi also had the same issue, and ended up adding a reply to the issue via email to bpo to work around the problem.

Perhaps the user creation should prohibit commas?

[rouilj]

See:

https://issues.roundup-tracker.org/issue2550921

for suggested change. Specifically https://issues.roundup-tracker.org/msg5936

[rouilj]

See also: #39. One of the solutions to that issue will also solve this issue.

[berkerpeksag]

@rouilj unfortunately changing userauditor will lead to the same UX issue as in #46. The user will still see the following message:

You will shortly receive an email to to confirm your registration. To complete the registration process, visit the link indicated in the email.

Would overriding RegisterAction and making something similar to:

https://github.com/psf/bpo-roundup/blob/68573d196f9a01786414d3b235252b9c857c3e08/roundup/cgi/actions.py#L1053-L1061

before:

https://github.com/psf/bpo-roundup/blob/68573d196f9a01786414d3b235252b9c857c3e08/roundup/cgi/actions.py#L1124

abort the registration with a user-friendly error message?

Currently, I have the following diff that prevents registering users with an invalid username:

diff --git a/detectors/userauditor.py b/detectors/userauditor.py
index d58eec5..eff098b 100644
--- a/detectors/userauditor.py
+++ b/detectors/userauditor.py
@@ -20,14 +20,25 @@
 #
 #$Id: userauditor.py,v 1.3 2006/09/18 03:24:38 tobias-herp Exp $
 
+import re
 import urlparse
 
+valid_username = re.compile(r'^[a-z0-9_.-]+$', re.IGNORECASE)
+
+
 def audit_user_fields(db, cl, nodeid, newvalues):
     ''' Make sure user properties are valid.
 
         - email address has no spaces in it
         - roles specified exist
     '''
+    if 'username' in newvalues:
+        if not valid_username.match(newvalues['username']):
+            raise ValueError(
+                'Username must consist only of the letters a-z (any case), '
+                'digits 0-9 and the symbols: ._-'
+            )
+
     if newvalues.has_key('address') and ' ' in newvalues['address']:
         raise ValueError, 'Email address must not contain spaces'
 
diff --git a/html/user.register.html b/html/user.register.html
index 65022e8..db00075 100644
--- a/html/user.register.html
+++ b/html/user.register.html
@@ -19,7 +19,10 @@
  </tr>
  <tr>
   <th class="required" i18n:translate="">Login Name</th>
-  <td tal:content="structure context/username/field">username</td>
+  <td>
+   <span tal:content="structure python:context.username.field(pattern='^[a-z0-9_.-]+$', required=True)">username</span>
+   Login name must consist only of the letters a-z (any case), digits 0-9 and the symbols: ._-
+  </td>
  </tr>
  <tr>
   <th class="required" i18n:translate="">Login Password</th>

But it would be nice to defer this step to Roundup without involving any client side validation.

[berkerpeksag]

In #39 (comment), you've proposed a similar solution, but I wonder if it's possible to trigger auditors without manually validating the input via self.client.parsePropsFromForm(create=1).

[rouilj]

Hi Berker: Sorry for the delay, time zones. In message <python/bugs.python.org/issues/35/667523969@github.com>, Berker Peksag writes:

@rouilj unfortunately changing `userauditor` will lead to the same UX issue as in #46. The user will still see the following message:

True, but the userauditor needs to change so the user can't change the username from the user details screen. you could add client side validation to the user.item.html template to mirror the server side userauditor validation. However in this case, failure report from the server are immediate so it's not as bad a UX. See: https://sourceforge.net/p/roundup/mailman/message/37075053/ for current and possibly future methods to validate registration requests.

> You will shortly receive an email to to confirm your registration. > To complete the registration process, visit the link indicated in > the email. Would overriding `RegisterAction` and making something similar to: https://github.com/psf/bpo-roundup/blob/68573d196f9a01786414d3b235252b9c857c3e08/roundup/cgi/actions.py#L1053-L1061 before: https://github.com/psf/bpo-roundup/blob/68573d196f9a01786414d3b235252b9c857c3e08/roundup/cgi/actions.py#L1124 abort the registration with a user-friendly error message?

Yes, that's sort of what is suggested in the email linked above. But you wrap RegisterAction. The wrapper validates the username against a regexp (could check to see if the username is already taken etc) using the fields from the form. Then calling RegisterAction.handle does all the hard work.

Currently, I have the following diff that prevents registering users with an invalid username: ```diff diff --git a/detectors/userauditor.py b/detectors/userauditor.py index d58eec5..eff098b 100644 --- a/detectors/userauditor.py +++ b/detectors/userauditor.py @@ -20,14 +20,25 @@ # #$Id: userauditor.py,v 1.3 2006/09/18 03:24:38 tobias-herp Exp $ +import re import urlparse +valid_username = re.compile(r'^[a-z0-9_.-]+$', re.IGNORECASE) + + def audit_user_fields(db, cl, nodeid, newvalues): ''' Make sure user properties are valid. - email address has no spaces in it - roles specified exist ''' + if 'username' in newvalues: + if not valid_username.match(newvalues['username']): + raise ValueError( + 'Username must consist only of the letters a-z (any case), ' + 'digits 0-9 and the symbols: ._-' + ) + if newvalues.has_key('address') and ' ' in newvalues['address']: raise ValueError, 'Email address must not contain spaces'

That looks good.

diff --git a/html/user.register.html b/html/user.register.html index 65022e8..db00075 100644 --- a/html/user.register.html +++ b/html/user.register.html @@ -19,7 +19,10 @@ </tr> <tr> <th class="required" i18n:translate="">Login Name</th> - <td tal:content="structure context/username/field">username</td> + <td> + <span tal:content="structure python:context.username.field(pattern='^[a-z0-9_.-]+$', required=True)">username</span> + Login name must consist only of the letters a-z (any case), digits 0-9 and the symbols: ._- + </td> </tr> <tr> <th class="required" i18n:translate="">Login Password</th> ```

That also looks good to me. Note I was doing the comments in the ticket on the fly (i.e. didn't test anything) but what you have looks like it should work.

But it would be nice to defer this step to Roundup without involving any client side validation.

A valid login name depends on the tracker. Some trackers allow email addresses as usernames. The no commas 'rule' is needed because usernames (e.g. in a nosy list) can be changed by a comma separated list of names. Embedded commas in the username confuses this UI interface. In theory, using a multi-select/options list would allow commas to work properly. Having both client side validation makes for a nicer UX. Having server side validation is required to prevent mischief. Your change to the template is the right way to do client side validation. The auditor provides the server side validation. Also one of the tickets against b.p.o in the tracker asks how to allow the user to log in using an email address. That is doable using an extension that wraps login (similar to wrapping RegisterAction). See https://wiki.roundup-tracker.org/LoginWithEmail Also in a follow on comment, you ask:

I wonder if it's possible to trigger auditors without manually validating the input via `self.client.parsePropsFromFor m(create=1)`.

Sadly no. The detector (auditor/reactor) mechanism requires an entry in the database. In the deferred (i.e. not instant) registration case the database interaction doesn't happen until after the email link is followed. This is what leads to the UX issues. That's what the second part of the email I referenced above discusses. Allowing a soft error makes it possible to reuse the detector logic in a nicer more polite way.

…

-- -- rouilj John Rouillard =========================================================================== My employers don't acknowledge my existence much less my opinions.

[berkerpeksag]

Sadly no. The detector (auditor/reactor) mechanism requires an entry in the database. In the deferred (i.e. not instant) registration case the database interaction doesn't happen until after the email link is followed.

Ah, yes, I see that ConfRegoAction calls confirm_registration:

https://github.com/psf/bpo-roundup/blob/68573d196f9a01786414d3b235252b9c857c3e08/roundup/roundupdb.py#L108-L134

I think this is a pretty common operation and it seems to be happening for pretty much all the fields in the registration form. I've just tested that I can get the following message even if I tried to register with an existing username:

You will shortly receive an email to to confirm your registration. To complete the registration process, visit the link indicated in the email.

What do you think about adding a __active__ field to the _user table, create user on database with __active__ = false and then after OTK step has completed, update __active__ to true?

Or can't we consider a user record with an active OTK record inactive/unverified since we immediately remove the props from OTK table after creation? https://github.com/psf/bpo-roundup/blob/68573d196f9a01786414d3b235252b9c857c3e08/roundup/roundupdb.py#L130 In short, is it doable to split the functionality in confirm_registration into two separate steps?

[berkerpeksag]

I've just pushed a slightly updated version of my inline patch in #35 (comment). We can continue discussing more general solutions here or somewhere else. Thanks!

[rouilj]

Hi Berker: In message <python/bugs.python.org/issues/35/667594221@github.com>, Berker Peksag writes:

> Sadly no. The detector (auditor/reactor) mechanism requires an entry > in the database. In the deferred (i.e. not instant) registration case > the database interaction doesn't happen until after the email link > is followed. Ah, yes, I see that `ConfRegoAction` calls `confirm_registration`: https://github.com/psf/bpo-roundup/blob/68573d196f9a01786414d3b235252b9c857c3e08/roundup/roundupdb.py#L108-L134

Correct. At that point the registration stored in temp storage is extracted and committed to the db where it will live forever more. Don't forget we can't delete things once they are created in the real database. Retired items still live in the db, just with the retired flag so they don't show up in normal searches.

I think this is a pretty common operation and it seems to be happening for pretty much all the fields in the registration form. I've just tested that I can get the following message even if I tried to register with an existing username: > You will shortly receive an email to to confirm your > registration. To complete the registration process, visit the link > indicated in the email.

Hence the need to do a query to the db during registration to see if that user already exists. Roundup 2.0.0 has native support for rejecting duplicate username registrations. Config.ini has a new registration_prevalidate_username option. Disabled by default since it can be used for username guessing.

What do you think about adding a `__active__` field to the `_user` table, create user on database with `__active__ = false` and then after OTK step has completed, update `__active__` to `true`?

The first spambot that comes along can fill up your user table. Once objects are created in the user table you have used up a primary key that can't be reused. On the other hand the OTK database can be deleted with minimal consequences. IIRC you had a problem with tons of bogus registrations. I integrated erik forsberg's mitigation method into the roundup core. In theory you could go in using sqlite, mysql etc. and modify the db underneath roundup's hyperdb. However there may be metadata and referential integrity issues. For example deleting a bogus user means you have to delete the entry from the journal for the creation of that user at the very least. So I can't recommend that.

Or can't we consider a user record with an active OTK record inactive/unverified since we immediately remove the props from OTK table after creation?

This is an area of the code I am not that familiar with. However IIRC the OTK record is indexed by the OTK. The username is stored in a blob. So there is no facility to index the pending OTC record by user. A scan of the OTK db is needed. The OTK database includes session identifiers, anti-csrf mitigation keys, client_nonces and other data as well so not exactly a quick simple scan.

https://github.com/psf/bpo-roundup/blob/68573d196f9a01786414d3b235252b9c857c3e08/roundup/roundupdb.py#L130 In short, is it doable to split the functionality in `confirm_registration` into two separate steps?

Well it is already split into two steps, pre-reg and complete-reg. That's kind of the problem you are reporting. If you enabled instant registration it would create the user in the same web session used to request an account. I would not recommend committing registration attempts to the user data store.