[Cookbook] Make registration_form follow best practices #4773

xelaris · 2015-01-05T03:30:46Z

Q	A
Doc fix?	yes
New docs?	no
Applies to	2.3+
Fixed tickets

javiereguiluz · 2015-01-05T07:44:41Z

cookbook/doctrine/registration_form.rst

@@ -45,7 +45,7 @@ You have a simple ``User`` entity mapped to the database::
        /**
         * @ORM\Column(type="string", length=255)
         * @Assert\NotBlank()
-         * @Assert\Length(max = 4096)
+         * @Assert\Length(max=4096)


Don't you think it's time to remove this constraint? Thinking about a password longer than 4,000 chars is ridiculous and the Security component already has this constraint hardcoded: https://github.com/symfony/symfony/blob/eee117285ae84b9aa859814d6d0cef9fd7f05baa/src/Symfony/Component/Security/Core/Encoder/BasePasswordEncoder.php#L23

There is a note about that in the unchanged text

Why the 4096 Password Limit?

Notice that the plainPassword field has a max length of 4096 characters. For security purposes (CVE-2013-5750), Symfony limits the plain password length to 4096 characters when encoding it. Adding this constraint makes sure that your form will give a validation error if anyone tries a super-long password.

I agree that a password longer than 4000 chars is ridiculous, but for me, it sounds like the constraint is still reasonable to provide a user friendly validation error. As I write this... a friendly user would never enter 4000 chars and a malicious user don't need a user friendly error message.

@xelaris to avoid getting an exception when trying to encode it (a malicious user should still receive a 400 status code rather than a 500)

@stof I agree, responding with 500 in this case would be neat. But if I am not wrong, without further changes the form with validation error will be shown along with status code 200. Anyway I agree the 4096 constraint makes sense to avoid 500.

While testing this, I saw an other issue with the example. Although the note mentions that the limit is relevant when it comes to encoding the password, in the current version, where the password is stored in plain text, the user gets a 500 response due to a PDOException, when entering a password longer than the table field but shorter than 4096.

What about expanding this chapter to store an encoded password? Alternatively either the column length should be set to match the constraint (e.g. 4096) or vice versa (e.g. 255). What do you think?

Wait, we are storing the plain password ? This must be fixed

I re-read the preamble of this entry. Thereby I become unsure about what the actual topic of this entry is. On the one hand it resides in the doctrine category and is about "extra fields whose values don't need to be stored in the database", on the other hand it's about a registration form and thus related to security.
The more I think about it... maybe this entry should be splitted into two entries. One to fulfill the preamble of the current entry and one to implement a comprehensive registration form like the headline promises. Therefore it requires another example/usecase for the doctrine/form related entry. The new entry about a registration form wouldn't need the embedded form, because the "terms accepted" can be ignored there. Assuming that the registration from would operate with the security system (e.g. an entity that implements the UserInterface of the Security component and using the recommended bcrypt encoder via security.yml) it would not be as "simple" as the current registration form. I think it would be nice to complement http://symfony.com/doc/current/cookbook/security/form_login_setup.html and http://symfony.com/doc/current/cookbook/security/entity_provider.html with the new entry about a registration from since the latter one already describes the required User implementing the UserInterface and the security.yml.
What are your opinions?

xelaris · 2015-01-07T22:59:53Z

PR has been updated to address the comments from @stof and @javiereguiluz. The password is now encoded before storing it into the database. This makes the chapter more security related than before. As mentioned in #4773 (comment) IMO splitting this chapter should be considered.

wouterj · 2015-01-16T15:53:23Z

Sorry, I'm a bit lost. Is this PR now ready to merge, or does it wait on comments?

xelaris · 2015-01-16T17:55:08Z

I think my last changes, which introduced the encoding of the password, should be reviewed. If the changes are approved, this PR could be merged IMO. Anyway, I think it would be a good idea to split the entry, but this could be done in a later PR if someone agrees with my opinion.

wouterj · 2015-01-16T17:57:52Z

cookbook/doctrine/registration_form.rst

@@ -10,13 +11,13 @@ database. For example, you may want to create a registration form with some
 extra fields (like a "terms accepted" checkbox field) and embed the form
 that actually stores the account information.

-The simple User Model
---------------------
+The simple User Entity


it's not your fault, but this should actually be: The Simple User Entity

And can you please add a placeholder for the old headline:

.. _the-simple-user-model: The Simple User Entity ----------------------

Simple could also be changed in the same way in the main headline (line 6).

xelaris · 2015-01-18T15:15:41Z

Thank you for the review @wouterj @xabbuh. I updated the PR.

xelaris · 2015-03-29T21:33:37Z

IMO this is ready to merge, isn't it?

javiereguiluz · 2015-03-30T06:42:08Z

@xelaris 👍 indeed it looks ready to be merged. Thanks!

xabbuh · 2015-03-30T18:05:37Z

cookbook/doctrine/registration_form.rst

@@ -36,7 +39,7 @@ You have a simple ``User`` entity mapped to the database::
        protected $id;

        /**
-         * @ORM\Column(type="string", length=255)
+         * @ORM\Column(type="string", length=255, unique=true)


Don't we need to also add a UniqueEntity constraint then?

Never mind, it's already part of the folded code above.

xabbuh · 2015-05-23T08:45:47Z

@wouterj As you added the "In Progress" label after the last change: What do you think needs to be done here?

xabbuh · 2015-12-17T18:53:10Z

Do we still have to do something here or is it ready to be merged?

wouterj · 2015-12-17T19:01:23Z

👍 (a rebase is needed though)

xabbuh · 2015-12-23T21:11:44Z

closing in favour of #6073

xabbuh · 2015-12-28T11:22:12Z

reopening here as #6073 was closed

xelaris · 2015-12-29T00:06:59Z

I rebased this PR and took a look into the overlap with #5214. Most of the changes made in this PR where already covered or obsoleted by #5214. There are only some tweaks remaining and a focus on encoding with bcrypt including the detailed example on how to configure the encoding algorithm.

Related to the comments made in #6073: The handling of the plain text password is still relevant, as the entity class holds both, the plain and the encoded password. In contrast to the first version of this PR, a password property was introduced in #5214 for persistence and the plainPassword gets stopped from being persisted, but kept relevant for form handling and validation. Actually there was a Registration DTO before #5214.

wouterj · 2016-02-07T10:55:56Z

👍 As far as I can see, this one looks ready to me.

xabbuh · 2016-02-07T13:29:21Z

Thank you @xelaris.

…xelaris) This PR was merged into the 2.3 branch. Discussion ---------- [Cookbook] Make registration_form follow best practices | Q | A | ------------- | --- | Doc fix? | yes | New docs? | no | Applies to | 2.3+ | Fixed tickets | Commits ------- b7ab3e4 [Cookbook] Tweaking registration_form (e.g. bcrypt)

javiereguiluz reviewed Jan 5, 2015
View reviewed changes

xelaris force-pushed the reg-form branch from 1ac8710 to 57d9cf2 Compare January 7, 2015 22:50

wouterj reviewed Jan 16, 2015
View reviewed changes

xelaris force-pushed the reg-form branch from 57d9cf2 to 3f9c0ec Compare January 18, 2015 15:03

xabbuh reviewed Mar 30, 2015
View reviewed changes

xelaris mentioned this pull request Apr 28, 2015

[WIP] Reworking most of the registration form: #5214

Merged

wouterj added the Status: Needs Work label May 4, 2015

javiereguiluz mentioned this pull request Dec 23, 2015

[Cookbook] Make registration_form follow best practices #6073

Closed

xabbuh closed this Dec 23, 2015

xabbuh reopened this Dec 28, 2015

xelaris force-pushed the reg-form branch from 3f9c0ec to 8f2b552 Compare December 28, 2015 23:55

[Cookbook] Tweaking registration_form (e.g. bcrypt)

b7ab3e4

xelaris force-pushed the reg-form branch from 8f2b552 to b7ab3e4 Compare December 29, 2015 17:22

wouterj added Status: Reviewed and removed Status: Needs Work labels Feb 7, 2016

xabbuh merged commit b7ab3e4 into symfony:2.3 Feb 7, 2016

xelaris deleted the reg-form branch October 31, 2022 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cookbook] Make registration_form follow best practices #4773

[Cookbook] Make registration_form follow best practices #4773

xelaris commented Jan 5, 2015

javiereguiluz Jan 5, 2015

xelaris Jan 5, 2015

stof Jan 6, 2015

xelaris Jan 6, 2015

stof Jan 6, 2015

xelaris Jan 6, 2015

xelaris commented Jan 7, 2015

wouterj commented Jan 16, 2015

xelaris commented Jan 16, 2015

wouterj Jan 16, 2015

xabbuh Jan 16, 2015

xelaris commented Jan 18, 2015

xelaris commented Mar 29, 2015

javiereguiluz commented Mar 30, 2015

xabbuh Mar 30, 2015

xabbuh Mar 30, 2015

xabbuh commented May 23, 2015

xabbuh commented Dec 17, 2015

wouterj commented Dec 17, 2015

xabbuh commented Dec 23, 2015

xabbuh commented Dec 28, 2015

xelaris commented Dec 29, 2015

wouterj commented Feb 7, 2016

xabbuh commented Feb 7, 2016

[Cookbook] Make registration_form follow best practices #4773

[Cookbook] Make registration_form follow best practices #4773

Conversation

xelaris commented Jan 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xelaris commented Jan 7, 2015

wouterj commented Jan 16, 2015

xelaris commented Jan 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xelaris commented Jan 18, 2015

xelaris commented Mar 29, 2015

javiereguiluz commented Mar 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented May 23, 2015

xabbuh commented Dec 17, 2015

wouterj commented Dec 17, 2015

xabbuh commented Dec 23, 2015

xabbuh commented Dec 28, 2015

xelaris commented Dec 29, 2015

wouterj commented Feb 7, 2016

xabbuh commented Feb 7, 2016