Email registration module usurps the registration page, generates a unique username, and only allows users to enter their email address.
In this case, the username constraint doesn't make any sense.
What would make more sense is to prevent the user from using their email address as their password.

However, password constraints don't have access to any form state values, because the password_policy validator doesn't send the form state along to the constraint checkers.

Feature Request

Give constraint checks access to form_state array or some other means of accessing submitted form values.

CommentFileSizeAuthor
#15 interdiff.txt603 bytesAaronBauman
#15 password_policy-form_state_constraint_check-2878918-15.patch14.16 KBAaronBauman
#13 interdiff-2878918-12-13.txt2.84 KBAohRveTPV
#13 password_policy-7.x-2.x-form_state_constraint_check-2878918-13.patch14.15 KBAohRveTPV
#12 interdiff.txt1.12 KBAaronBauman
#12 password_policy-form_state_constraint_check-2878918-12.patch13.76 KBAaronBauman
#9 email.inc_.txt1.76 KBAaronBauman
#9 password_policy-form_state_constraint_check-2878918-9.patch13.08 KBAaronBauman
#5 email.inc_.txt1.62 KBAaronBauman
#5 password_policy-form_state_constraint_check-2878918-5.patch2.38 KBAaronBauman
#3 password_policy-form_state_constraint_check-2878918-3.patch2.01 KBAaronBauman
#2 password_policy-form_state_constraint_check-2878918.patch1.73 KBAaronBauman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aaronbauman created an issue. See original summary.

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented
FileSize
password_policy-form_state_constraint_check-2878918.patch1.73 KB

This kind of check would require $form_state as an argument to Constraint::check like the attached patch.

Is there any good reason NOT to do this?
Otherwise I have to do some kind of crazy static-cache based form cache to look at any other fields on the form.

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented
FileSize
password_policy-form_state_constraint_check-2878918-3.patch2.01 KB
AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented
Title: Email constraint » Email constraint & access to form state
Issue summary: View changes
AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented
FileSize
password_policy-form_state_constraint_check-2878918-5.patch2.38 KB
email.inc_.txt1.62 KB

One more.
This one is working locally, with an example constraint (email.inc.txt attachment).

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

Without looking at the code, I'm wondering if we could get rid of $account and just use $form_state.

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented

seems like it, since $account is available in $form_state
already the username and drupal_strength constraints are looking directly at $_POST, which seems like a hacky workaround.

the tricky bit (of course) seems like the ajax callback.

shall I work on a proper patch?

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

Sure. Thanks for looking into this problem.

It looks like 7.x-1.x has the same limitation. We probably shouldn't change the Constraints API there, though, since 7.x-1.x has been a full release for years, and people probably have custom constraints written against its Constraints API.

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented
FileSize
password_policy-form_state_constraint_check-2878918-9.patch13.08 KB
email.inc_.txt1.76 KB

There are 4 existing constraints making use of account information:
1. delay
2. past_passwords
-- both these constraints need the account id to fetch user-specific information. This feature is updated thus:

     global $user;
-
-  if (!$account) {
-    $account = $user;
-  }
+  $account = empty($form_state['complete form']['#user'])
+    ? $user 
+    : $form_state['complete form']['#user'];

3. username
4. drupal_strength
-- both these constraints rely only on the username string value, and I DRYed this up by putting the helper function into password_policy.module:

+ * Helper function for constraint methods to fetch username from $form_state.
+ */
+function _password_policy_get_form_state_username($form_state) {
+  $username = '';
+  $account = $form_state['complete form']['#user'];
+  if (isset($form_state['input']['name'])) {
+    // The username will not be displayed, so there is no need to filter it
+    // with check_plain() or filter_xss() as suggested by Coder.
+    // @ignore security_17
+    $username = rawurldecode($form_state['input']['name']);
+  }
+  elseif ($account->uid > 0) {
+    // The username input will not be present on the same form as the password
+    // input in some configurations (e.g., sites using Password Tab module). So
+    // fall back to retrieving the username from the user object.
+    $username = $account->name;
+  }
+  return $username;
+}
+
+/**

All other constraints ignore the $account argument completely.

Finally, the ajax callback builds $form_state with drupal_build_form():

+    $form_state = array();
+    drupal_build_form($_POST['form_build_id'], $form_state);

Also updated the example email constraint.

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: password_policy-form_state_constraint_check-2878918-9.patch, failed testing.

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented
Status: Needs work » Needs review
FileSize
password_policy-form_state_constraint_check-2878918-12.patch13.76 KB
interdiff.txt1.12 KB

Updated test and call to failMessages()

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
FileSize
password_policy-7.x-2.x-form_state_constraint_check-2878918-13.patch14.15 KB
interdiff-2878918-12-13.txt2.84 KB 
$ drupalcs .

FILE: ...ll/modules/password_policy/includes/PasswordPolicyConstraint.inc
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 69 | ERROR | Parameter $form_state is not described in comment
 74 | ERROR | Doc comment for parameter $account does not match
    |       | actual variable name $form_state
----------------------------------------------------------------------


FILE: ...l7/sites/all/modules/password_policy/includes/PasswordPolicy.inc
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 108 | ERROR | Parameter $form_state is not described in comment
 113 | ERROR | Doc comment for parameter $account does not match
     |       | actual variable name $form_state
----------------------------------------------------------------------

FILE: ...7/sites/all/modules/password_policy/plugins/constraint/delay.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 51 | ERROR | [x] Whitespace found at end of line
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ll/modules/password_policy/plugins/constraint/past_passwords.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 42 | ERROR | [x] Whitespace found at end of line
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Fixed these.

$ drupalcsp .

FILE: ...drupal7/sites/all/modules/password_policy/password_policy.module
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 311 | WARNING | Do not use the raw $form_state['input'], use
     |         | $form_state['values'] instead where possible
 315 | WARNING | Do not use the raw $form_state['input'], use
     |         | $form_state['values'] instead where possible
----------------------------------------------------------------------

$values doesn't have the needed values. Added ignore directives for these warnings.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

No problems in my ad hoc testing (just the username constraint, though).

Two concerns with this line:

drupal_build_form($_POST['form_build_id'], $form_state);

1. What would happen if someone made a POST request with a fraudulent form_build_id? Are there any security implications to this? I'm not sure offhand, will plan to look into.
2. Will this hurt performance on a busy website? drupal_build_form() would be called with every few keystrokes as a user enters their password.

Not so worried about #2, we can find out through usage. #1 more concerning.

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented
FileSize
password_policy-form_state_constraint_check-2878918-15.patch14.16 KB
interdiff.txt603 bytes

1. What would happen if someone made a POST request with a fraudulent form_build_id? Are there any security implications to this? I'm not sure offhand, will plan to look into.

2 points on this:
1. this should be passing form_id, with form_build_id in the form_state, to drupal_build_form
2. this is how drupal_get_form (and thence drupal_build_form) work already, so i don't think it opens any security risk that isn't already present. it's a long, unique hash string which is difficult to forge, tied to a short-term cache identifier.

2. Will this hurt performance on a busy website? drupal_build_form() would be called with every few keystrokes as a user enters their password.

Definitely could have an impact, but I'd say no more than it's already having.
I think bootstrapping Drupal will have the biggest hit, and loading a cached form array will be minimal in comparison.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

This seems to let a user build any form they want by specifying its form ID as a POST parameter. I'm worried about the implications of that. Instead of user_register_form, for instance, they could cause an administration form to be built. I don't see anywhere in Drupal core where the user has the ability to specify the form ID passed to drupal_build_form().

I'll keep looking at the code to see if I can convince myself it is safe.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

Probably beyond the scope of this issue, but I suspect the module should be using Drupal's AJAX API instead of directly doing POSTs.

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented

This seems to let a user build any form they want by specifying its form ID as a POST parameter

That would definitely be a problem.

I may have time tomorrow to look at #ajax'ing the form (in a separate issue).

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented
Status: Needs review » Postponed
Related issues: +#2879489: #ajax-ify password callback

Let's mark this postponed on the outcome of #2879489: #ajax-ify password callback