Security Questions

Removing review bonus tag, you did not do manual reviews as required in #1410826: [META] Review bonus.

1. You're defining a very generic constant:

define('CACHE_DISABLED', 0);

Prefix your constants with your module name.

2. When you go to admin/config/people/security_questions, you see the settings (just 1 field). More sensible IMO would be to have the questions tab be the default: admin/config/people/security_questions/questions.

3. In hook_menu:

  $items['user/%/security_questions'] = array(
  $items['user/%/security_questions/edit/%'] = array(

The % can be %user and then you can remove this nasty piece of code from security_questions_user_edit_form:

  $url = explode('/', request_uri());
  $uid = $url[2];

You can then change the function definition to security_questions_user_edit_form($form, &$form_state, $user) if you add a page arguments element to the menu item in hook_menu. See http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...

4. Overriding $items['user']['page callback'] in hook_menu_alter is very brutal and security_questions_user_page is a mess. If you're logged in, it logs you in... Weird...

Check out user_page() and user_view_page() (in modules/user/user.pages.inc) to see what Drupal does with user.

...

Now it's failing for me completely. No idea what happened, but Chrome returns a net::ERR_EMPTY_RESPONSE error for me when the module is enabled. Authenticated and anonymous alike. When I disable the module with drush, everything works again.

Obviously this is not gonna work:

function security_questions_user_page() {
  global $user;
  if ($user->uid > 0) {
    user_login($form = NULL, &$form_state);

You're passing $form as NULL. $form doesn't exist. $form_state doesn't exist.

What is it you're trying to do here?

If I comment out the hook_menu_alter, the crashes stop (but the module doesn't do its login thing).

There's something very wrong. I'm pretty sure it's in security_questions_login. Maybe calling drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL)... I'm not sure.

Did you take it from Drupal 7's IP Login? Do you know what it does? Don't mindelessly copy and paste.

I give up. It's up to you to debug and fix. Drupal doesn't save any error messages, so it happens before watchdog is ready, I think. All I get is:

Deprecated function: Call-time pass-by-reference has been deprecated in drupal_load() (line 1114 of /var/www/quadrupal7/includes/bootstrap.inc).

It would be best if you could fit this module into existing user login. You can form alter the login form and remove elements (step 1: remove password) or make them #type => hidden (step 2: hide username). You can add validation to check the security question. I don't think overriding this much and calling drupal_bootstrap is necessary.

It's well possible without overriding so much and calling bootstrap functions etc.

This might help:

<?php

/**
 * Implements hook_form_FORM_ID_alter() for user_login().
 */
function security_questions_form_user_login_alter(&$form, &$form_state, $form_id = 'user_login') {
	// Step 1: enter username.
	if ( empty($form_state['security_questions']) ) {
		$form['#validate'] = array('security_questions_user_login_validate_name');

		// Remove password field and submit handlers
		unset($form['pass'], $form['#submit']);
	}

	// Step 2: enter password + security answer.
	else {
		// Hide username.
		$form['name']['#type'] = 'hidden';

		// Retrieve account from form_state (put there by our validation function).
		$account = $form_state['security_questions']['account'];

		// Get a random question for this user.
		$question = security_question_get_random_question($account);

		// Show answer element.
		$form['security_question'] = array(
			'#type' => 'fieldset',
			'#title' => 'Security question',

			// Don't store this in the form. Store it in the session or the database.
			'question_id' => array(
				'#type' => 'hidden',
				'#value' => $question->security_question_id,
			),

			'question' => array(
				'#type' => 'item',
				//'#title' => 'Question',
				'#markup' => '<div>' . check_plain($question->security_question) . '</div>',
			),
			'security_answer' => array(
				'#type' => 'textfield',
				'#title' => 'Answer',
				'#required' => TRUE,
			)
		);

		// Add anwser validation.
		// The other validations (from user.module) exist as well. Answer validation is extra.
		$form['#validate'][] = 'security_questions_user_login_validate_answer';
	}
}

/**
 * Validation handler for security_questions_form_user_login_alter().
 */
function security_questions_user_login_validate_name($form, &$form_state) {
	$username = $form_state['values']['name'];

	$account = user_load_by_name($username);

	if ( !$account || !$account->status ) {
		form_set_error('name', "This user doesn't exist or is disabled.");
	}
	else {
		drupal_set_message("You're almost logged in. Just answer this question.", 'warning');

		// Save the account for the next step.
		// @see security_questions_form_user_login_alter()
		$form_state['security_questions']['account'] = $account;

		// We're not done yet! Even though technically this form will submit successfully.
		// Rebuild the form and alter it differently: with a question & answer.
		// This option will also ensure the form handler doesn't redirect, which would
		// mean we lose all form state and POST data.
		$form_state['rebuild'] = TRUE;
	}
}

/**
 * Validation handler for security_questions_form_user_login_alter().
 */
function security_questions_user_login_validate_answer($form, &$form_state) {
	$errors = form_get_errors();

	if ( !$errors ) {
		// Get uid from form state. It was put there by a validation handler in user.module.
		$uid = $form_state['uid'];

		// Get question from form.
		$question_id = $form_state['values']['question_id'];

		// Get answer from database.
		$params = array($uid, $question_id);
		$answer = db_query('SELECT user_answer FROM {security_questions_answers} WHERE uid = ? AND security_question_id = ?', $params)->fetchObject();

		$user_answer = drupal_strtolower(trim($form_state['values']['security_answer'], ' .!'));
		$db_answer = drupal_strtolower(trim($answer->user_answer, ' .!'));
		if ( $user_answer != $db_answer ) {
			form_set_error('security_answer', "That's not it...");
		}
	}
}

/**
 * Returns a user's random question from the database.
 */
function security_question_get_random_question($account) {
	$query = db_query('SELECT q.* FROM {security_questions} q, {security_questions_answers} a WHERE a.uid = ? AND q.security_question_id = a.security_question_id ORDER BY RAND() LIMIT 1', array($account->uid));
	$question = $query->fetchObject();
	return $question;
}

?>

Don't copy and paste it. Understand it and use it.

MANUAL REVIEW
--------------
1. .module Line 233 and others: Include the $i variable in the t() call using a placemarker.

2. .module Line 218: You will want to send the question through these functions t(check_plain()).

3. .module Line 298: Send the question also through t(). Make sure all text that is displayed goes though this function.

AUTO REVIEW
------------

1. Coder returned quite a few minor errors. Please fix these.

2. PAReview online tool didn't return any results.

Changed status to needs work.

Code looks good!

1.

You forgot a few check_plains. I won't spoil it. What I usually do is enter something with <textarea> in the name (question and answer both) and then check everywhere it's used.

2.

If I don't have a security question set, I can log in without answering anything. Is that intended?

3.

If I have 1 of 2 required of 5 available questions answerered, I can't answer my second. Is that intended?

4.

On admin/config/people/security_questions you put the entire table inside a #markup element. Nothing wrong with that, but what if I want to write a module to add a column to that table? E.g. with the number of users that have that question answered. You should allow other modules to do that. Two methods I know:

1. drupal_alter $table before passing it to theme_table
2. put everything in separate elements and theme the entire form + table in a dedicated theme function (which is also preprocessable by other modules and themes)

5.

Why do you use drupal_write_record and not db_insert?

6.

It's always good to put (submit) buttons in an "actions" form element.

  $form['security_question_user_edit_submit'] = array(
    '#type' => 'submit',
    '#value' => t("Change answer"),
  );

Like so:

  $form['actions'] = array('#type' => 'actions');
  $form['actions']['submit'] = array(
    '#type' => 'submit',
    '#value' => t("Change answer"),
  );

7.

Just FYI. The result of a db_query is an Iterable, which means you can read its results with foreach and no ->fetchObject() is required.

$stuffs = db_query(...);
foreach ($stuffs as $stuff) {

Just curious:

What does this do?

/**
 * Implements hook_user_login().
 */
function security_questions_user_login(&$edit, $account) {
  $question = security_questions_get_random_question($account->uid);
  if (empty($question)) {
    drupal_get_form('security_questions_answer_form');
  }
}

8.

Oops:

        form_set_error('security_question_id_' . $i, 'Please select a questions that you have not yet picked.');

Whenever you type something in English, it must be translated. Sometimes indirectly (menu item titles) and sometimes directly (this, form element titles, help texts etc).

3. I meant that if somehow I have 1 of 2 required questions answered (somehow), I can't answer the 2nd required question, so I'll only have 1 even though 2 are actually required. (The "somehow" is important: a user should never have too few qustions answered.)

I'll check out the rest tonight.

1.

This is weird (and wrong):

function security_questions_list_user($uid) {
  $result = db_query(...)->fetchAll(); // << might be an empty Array
  if ($result) {
    // return something
  }
  // do nothing??
}

If a user has no questions answered, the page is blank (there's no return value, so Drupal renders nothing).

I see from the diff (bff6be5..71a4d87) you've explicitly removed this:

-  // If the user has not answered questions, direct them to the add form.
-  else {
-    drupal_goto('user/' . $uid . '/security_questions/add');
-  }

Why? That was good, wasn't it?

2.

No underscore needed (better if you don't):

    drupal_alter('_security_questions_user_list', $table);

3.

This is dangerous:

$query = db_query('SELECT COUNT(security_question_id)  FROM {security_questions_answers} a WHERE a.uid = :uid', array(':uid' => $account->uid))->fetchAssoc();
$count = $query['count(security_question_id)'];

You're counting on the output column to be called "count(security_question_id)" while you call it "COUNT(security_question_id)" in the query. To fetch a single column value, you can use db_query()->fetchField().

See this comment on the Drupal DBAL page.

PS. Happens in security_questions_user_login_questions_validate too. And in security_questions_user_login_questions_submit too.

4.

From element somewhere in security_questions_form_user_login_alter (I'm reading the diff):

        '#title' => 'Security questions',

Translate form element labels. Think about every English literal you type: where will it be translated? Think about every variable you 'print': where will it be escaped/encoded?

5.

        $output = theme('table', $table);

        $form['security_questions']['answered'] = array(
          '#type' => 'item',
          '#markup' => t($output), // w0000t?
        );

Let's not translate a full HTML table =)

On a side note: don't ever put variables inside t() directly: t($something). Reason: Drupal translation server reads those literals from code to be translated by users. Every t() call starts with t(' or t(".

Related:

          while ($q = $questions->fetchObject()) {
            $options[$q->qid] = t($q->sc);
          }

You're translating a variable, which is 'bad' AND it's a user string. You sure you want to translate those? (It might. If you force admins to insert questions in English and users can read them in another language, it might be necessary.)

6.

  // Load the user via their username.
  $account = user_load_by_name($username);

  // Check to see if the user has already answered the security questions.
  $question = security_questions_get_random_question($account->uid);
  if (!$account || !$account->status) {

What happens if the user doesn't exist? "Notice: Trying to get property of non-object in security_questions_user_login_validate_name()" You don't need $question before checking the user. You can call it after the if.

7.

You might want to punt this in a helper function:

     $user_answer = drupal_strtolower(trim($form_state['values']['security_answer'], ' .!'));
     $db_answer = drupal_strtolower(trim($answer->user_answer, ' .!'));

The trim and drupal_strtolower I mean. The DRYer, the better.

@rudiedirkx
please don't put each issue you find into a new comment, this makes the issue long and confusing

1.

There's something deeply wrong again... The site keeps crashing if the module is enabled. I'm thinking it has to do with the by reference arguments. Do you know what a &$var does when it's not in a function argument?

Specifically: you're calling drupal_retrieve_form:

$form += drupal_retrieve_form('security_questions_user_answer_form', &$form_state);

With & vars. Why? If you want form_state to be (able to be) altered, you don't have to change the input, only the function argument (in drupal_retrieve_form, which you can't change).

Removing the & in three locations fixes the problem. Maybe these pages help.

(You might not be feeling this, because I'm running PHP 5.3 on Linux and you're not?)

2.

This feels a little strange to me:

t(check_plain($result->security_question))

This allows translated strings to contain HTML, but not untranslated (English?) strings. More sensible IMO would be the other way around: translate first and then html encode. But it's up to you obviously. It's not wrong.

3.

Inside security_questions_user_insert, you might want to check if the answers exist in $edit: $edit['security_question_id_' . $i]. You don't know where a user insert is called or who might've altered the user form.

The last two aren't really bugs. The first one is.

regarding
t(check_plain($result->security_question))
it is wrong in fact as t() already checkplain's all input of the first parameter.

Nice reviews you gave to those other people!

Nice module. This is definitely something needed and will probably be well used by the community. This was quite a lot to tackle, and I'm impressed with how far you've dived in.

You have shown you have done your due diligence to research existing modules, and it is great that you are explaining this on the project page. I think that there can be an argument made to help improve the riddler project rather than make a new module, but an argument can also be made that it is perfectly valid to have something similar but different. I also don't believe this should be a blocker for being a contributor because what we're really trying to figure out here is if you understand the Drupal community and the standards we all try to follow. Clearly, you are following the community standards by having done prior research on other modules.

We shouldn't let design decisions and bugs be a blocker for contributor approval. Most certainly, understanding how the translation system works is really important to grasp, but that is a Drupal-specific thing. Programming-specific critiques (hmm, this would silently fail) shouldn't be blockers. I think it is great that you have been working so hard to reflect upon these critiques and improve your work. You'll definitely get a lot of these critiques in your project issue queues!

Good README.

I agree with rudiedirkx that you really should be using %user in your hook_menu() call instead of just % and then exploding the path and extracting the UID. Otherwise, you get the whole $user object available to you in security_questions_list_user() which is awesome. You'll love it. Not a blocker, though.

Love your liberal use of comments.

I saw a couple bugs, but not blockers. This is a big thing and will need a bunch of debugging. You won't want to make an official release of this until it is completely debugged. Hopefully more eyes will get involved and help you out with it!

So, I think chertzog is doing a great job and has demonstrated he is a valuable asset to the Drupal contributor community. Let's get you on board :)

Hi chertzdog, you are doing a great job, and especially thank you for doing other reviews, and you'll definitely be brought on board but i'd like to see the module a little more polished. Leaving at RTBC because i think you should get the ability to promote projects to full, but it doesn't seem urgent as this one isn't ready yet -- at least i get two functionally-major bugs when trying out the 7.x-1.x branch:

1. I can create questions but not answers ;-)
2. And it doesn't actually ask me a question before signing in

I'm thinking there might have been some regressions while doing fixes, anyhow, your style is great and looking forward to it being ready for release!

Thanks for your contribution, chertzog! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

Congratz @chertzog! I knew it, ofcourse. Let's fill that issue queue wil cool feature requests =)