Add validation constraints to user.settings
Closes #3436164
Merge request reports
Activity
added 163 commits
-
8934767a...e5ce10b4 - 159 commits from branch
project:10.3.x - ea824359 - constraints
- cb30acfd - fix tests
- 2aa18abe - partial fix push
- a961a6c4 - remove unecessary
Toggle commit list-
8934767a...e5ce10b4 - 159 commits from branch
51 - 'visitors' 52 - 'admin_only' 53 - 'visitors_admin_approval' 44 54 cancel_method: 45 55 type: string 46 56 label: 'When cancelling a user account' 57 nullable: true 58 constraints: 59 NotBlank: 60 allowNull: true 47 61 password_reset_timeout: 48 62 type: integer 49 63 label: 'Password reset timeout' 64 constraints: 65 Range: 66 min: 0 changed this line in version 5 of the diff
43 45 label: 'Who can register accounts?' 46 # Choices are derived from the constants. 47 # @see \Drupal\user\UserInterface 48 constraints: 49 Choice: 50 choices: 51 - 'visitors' 52 - 'admin_only' 53 - 'visitors_admin_approval' 44 54 cancel_method: 45 55 type: string 46 56 label: 'When cancelling a user account' 57 nullable: true 58 constraints: 59 NotBlank: 60 allowNull: true changed this line in version 6 of the diff
44 54 cancel_method: 45 55 type: string 46 56 label: 'When cancelling a user account' 57 nullable: true 58 constraints: 59 Choice: 60 - user_cancel_block 61 - user_cancel_block_unpublish 62 - user_cancel_reassign 63 - user_cancel_delete 64 NotBlank: 65 allowNull: true 47 66 password_reset_timeout: 48 67 type: integer 49 68 label: 'Password reset timeout' 69 # A reasonable time would be 10 minutes. changed this line in version 8 of the diff
44 54 cancel_method: 45 55 type: string 46 56 label: 'When cancelling a user account' 57 nullable: true 58 constraints: 59 Choice: 60 - user_cancel_block 61 - user_cancel_block_unpublish 62 - user_cancel_reassign 63 - user_cancel_delete 64 NotBlank: 65 allowNull: true 47 66 password_reset_timeout: 48 67 type: integer 49 68 label: 'Password reset timeout' 69 # A reasonable time would be 10 minutes. -
This can be extended a bit to give reason for this 10 minutes.
Edited by Narendra Singh Rathore changed this line in version 8 of the diff
41 43 register: 42 44 type: string 43 45 label: 'Who can register accounts?' 46 # Choices are derived from the constants. 47 # @see \Drupal\user\UserInterface 48 constraints: 49 Choice: 50 choices: 51 - 'visitors' 52 - 'admin_only' 53 - 'visitors_admin_approval' 44 54 cancel_method: 45 55 type: string 46 56 label: 'When cancelling a user account' 57 nullable: true changed this line in version 6 of the diff
-
Removed, previously necessary but was fixed with upstream
Edited by omkar podey
48 constraints: 49 Choice: 50 choices: 51 - 'visitors' 52 - 'admin_only' 53 - 'visitors_admin_approval' 44 54 cancel_method: 45 55 type: string 46 56 label: 'When cancelling a user account' 57 nullable: true 58 constraints: 59 Choice: 60 - user_cancel_block 61 - user_cancel_block_unpublish 62 - user_cancel_reassign 63 - user_cancel_delete - Comment on lines +59 to +63
Should we use
Callbackhere? as user_cancel_methods() can have additional values also. See hook_user_cancel_methods_alter()Edited by Narendra Singh Rathore changed this line in version 6 of the diff
41 43 register: 42 44 type: string 43 45 label: 'Who can register accounts?' 46 # Choices are derived from the constants. 47 # @see \Drupal\user\UserInterface changed this line in version 11 of the diff
49 Choice: 50 choices: 51 - 'visitors' 52 - 'admin_only' 53 - 'visitors_admin_approval' 44 54 cancel_method: 45 55 type: string 46 56 label: 'When cancelling a user account' 57 constraints: 58 Choice: 59 callback: 'Drupal\user\Plugin\Validation\Constraint\UserCancelMethodsConstraints::getCancelMethodChoices' 47 60 password_reset_timeout: 48 61 type: integer 49 62 label: 'Password reset timeout' 63 # After a bit of research, 20 minutes seemed appropriate as it 64 # gives enough time to even access a mail account with MFA. - Comment on lines +63 to +64
"After a bit of research" is fine in a d.o comment, but won't be able to land in Drupal core
More importantly: what if a site does not use MFA? What if an existing site that is very conscious of security has set this to e.g. 5 minutes? Or even 1 minute? With the https://www.drupal.org/project/user_pwreset_timeout module, that's easy to set.
I think that in this case, a minimum of 1 minute (i.e.
60) may be necessary to avoid disrupting real-world websites. Perhaps even just 1 second, because https://git.drupalcode.org/project/user_pwreset_timeout/-/blob/8.x-1.x/user_pwreset_timeout.module?ref_type=heads specifies that as the minimum and hence it's likely some sites will be using this…(We could open up a follow-up issue to make it stricter to improve security though — but that should then be a follow-up issue which has an update hook to increase the minimum.)
changed this line in version 9 of the diff
-
[#3441772] follow-up
2 3 namespace Drupal\user\Plugin\Validation\Constraint; 4 5 /** 6 * Gets the cancel methods. 7 */ 8 class UserCancelMethodsConstraints { 9 10 /** 11 * Gets the cancel methods. 12 */ 13 public static function getCancelMethodChoices():array { 14 return user_cancel_methods(TRUE); 15 } 16 17 } - Comment on lines +8 to +17
Wow, you really have to touch on positively ancient code here!
Rather than introducing a new API surface here that we'd then have to maintain, I think it might be better to instead use the strategy that @phenaproxima used in [#3437325], where he introduced the
CountryCodesconstraint, which wraps theChoiceconstraint.
changed this line in version 9 of the diff
48 constraints: 49 Choice: 50 choices: 51 - 'visitors' 52 - 'admin_only' 53 - 'visitors_admin_approval' 44 54 cancel_method: 45 55 type: string 46 56 label: 'When cancelling a user account' 57 constraints: 58 UserCancelMethod: [] 47 59 password_reset_timeout: 48 60 type: integer 49 61 label: 'Password reset timeout' 62 # Any higher value could break sites using a lower value. 63 # Harden this in a follow-up. - Comment on lines +62 to +63
It's a tricky line to walk, but this first line is the kind of comment we don't generally put into Drupal core.
Not a tricky line but a hard rule: the second line refers to a follow-up … which does not exist. Every time we do this, we must A) create the issue, B) point to it using
@see <URL>.99% ready!
changed this line in version 12 of the diff
