Feedback on the package

[Stéphane Raimbault]

Hi,

I give a try to the TurboGears extension named registration and I
would give you some feedback if you're interested.

1. I find the name confusing, something like TurboRegistration will be
more convenient for me (ok it's minor :)

2. registers_controllers.py, line 251, recieve -> receive

3. I don't think it's a good idea to create the registration tables at
startup, it's an important operation I will better to inform the user
to create the tables.

4. I don't want to depend on registration.ormmanager, it's really
important for me to reduce the dependencies and your package already
provides source code for SA or SQLObject in the model so can you
remove this dependency?

5. There is a major security problem in the password reset, an user
can reset the password for someone else. To secure this step, it's
easy to add an activation key.

6. The password is stored in clear into registration_pending_user!

7. Many strings aren't marked for translations.

8. it useless to get the following parameters with TurboMail enabled:
def __init__(self):
super(UserRegistration, self).__init__()
random.seed()
self.hash_salt = ''.join([random.choice(string.printable) for
i in range(20)])
self.smtp_server = config.get('registration.mail.smtp_server',
'localhost')
self.smtp_port =
config.get('registration.mail.smtp_server_port', 25)
self.smtp_username =
config.get('registration.mail.smtp_server.username', None)
self.smtp_pw =
config.get('registration.mail.smtp_server.password', None)

Regards,
Stéphane

[Patrick Lewis]

On Jun 23, 10:36 am, Stéphane Raimbault <stephane.raimba...@gmail.com>
wrote:

> Hi,
>
> I give a try to the TurboGears extension named registration and I
> would give you some feedback if you're interested.

Yes, I am interested. Thanks for your good comments.

>
> 1. I find the name confusing, something like TurboRegistration will be
> more convenient for me (ok it's minor :)

I agree. Unfortunately, when I named it, I thought TurboWhatever was
reserved for official turbogears projects, and this isn't one. So, it
got named registration, and now I'm stuck with it.

>
> 2. registers_controllers.py, line 251, recieve -> receive

I apologize for this, but please be sure that you are working with the
most recent version of registration (0.4). Many people have reported
problems with getting the 0.4 release using easy_install, and I
haven't been very good about following up with why that is. I promise
to do so, but in the meantime please try:

easy_install http://pypi.python.org/packages/source/r/registration/registration-0.4.tar.gz

to get the latest version.

>
> 3. I don't think it's a good idea to create the registration tables at
> startup, it's an important operation I will better to inform the user
> to create the tables.

The auto-creation has been removed in the 0.4 release.

>
> 4. I don't want to depend on registration.ormmanager, it's really
> important for me to reduce the dependencies and your package already
> provides source code for SA or SQLObject in the model so can you
> remove this dependency?

I understand, but don't see making this change. Changing it would make
the controller code too hard for me to read and maintain. If I don't
have this abstraction layer, I have to have specific CRUD logic for
each orm in the controller template, and cheetah template IF
statements around them. Then, I have to test each path using both
ORMs. With the abstraction layer, I can pretend there is only one
ORM, and can test the abstraction layer.

To give you a sense of what it would look like without the ormmanager,
look here at all the #if identity=='sqlobject' branches.

http://tinyurl.com/6r3u64

>
> 5. There is a major security problem in the password reset, an user
> can reset the password for someone else. To secure this step, it's
> easy to add an activation key.

Yes, I agree (when the policy is 'reset'). I will change this asap.

>
> 6. The password is stored in clear into registration_pending_user!

I can change this too (for hashed passwords). It's complicated a bit
because you need it in the clear for cases when identity doesn't hash
the passwords. But you are right, it is lazy if identity is using
hashes. Just to be clear, the security risk is that if someone has
access to your database (or backups), they have access to the pending
user passwords.

>
> 7. Many strings aren't marked for translations.

If you don't mind, I could use some specific examples here. I've
looked through the controller code, and can see some exception strings
that aren't marked, but not much else. Or are you referring to kid
templates?

>
> 8. it useless to get the following parameters with TurboMail enabled:
> def __init__(self):
>         super(UserRegistration, self).__init__()
>         random.seed()
>         self.hash_salt = ''.join([random.choice(string.printable) for
> i in range(20)])
>         self.smtp_server = config.get('registration.mail.smtp_server',
> 'localhost')
>         self.smtp_port =
> config.get('registration.mail.smtp_server_port', 25)
>         self.smtp_username =
> config.get('registration.mail.smtp_server.username', None)
>         self.smtp_pw =
> config.get('registration.mail.smtp_server.password', None)

Useless, yes. But it's only useless once. :-) Ok, it's not much to
check for turbomail here.

Thanks again for the feedback. I'm actually a little embarrassed about
the password reset bug, but I'll live.