[D7] Httplock

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi mister.koz, thank you for share this module.

Would be great if exist a flag to enable and disable all access checking, instead of having to uninstall the module to disable the feature.

Custom Review:

httplock.module | LINE 19: 'page arguments' => array('httplock_form')
httplock_form.inc | LINE 13: function httplock_form($form, &$form_state) {
I advise you to rename the function to form a more intuitive name, such as: httplock_settings_form

httplock.module | LINE 61
httplock.module | LINE 102
You are using the same verification in both functions, you can create a function with this verification, and avoid duplicated code.

httplock.module | LINE 191

/**
 * Httplock success method.
 */
function httplock_success() {
  // This is an empty method stub, used for testing.
}

I believe that this function is not needed, since it is not doing anything

httplock.module | LINE 202
Use a constant instead of a directly number. Clean code ;)

httplock.module | LINE 209
httplock.module | LINE 210
httplock.module | LINE 233
httplock.module | LINE 234
You can use a core function to add http header: https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drup...

httplock.module | LINE 213
You can use drupal_exit();

httplock_form.inc | LINE 133
You can store this in a function.

httplock_form.inc | LINE 138
Remove this 'echo'.

Code Standards Review:
FILE: httplock.drush.inc
--------------------------------------------------------------------------------
FOUND 25 ERROR(S) AFFECTING 25 LINE(S)
--------------------------------------------------------------------------------
19 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
28 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
31 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
40 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
43 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
52 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
55 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
65 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
75 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
85 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
95 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
104 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
107 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
116 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
119 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
129 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
139 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
148 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
151 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
160 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
163 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
172 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
175 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
184 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
187 | ERROR | Multi-line array contains a single value; use single-line array
| | instead
--------------------------------------------------------------------------------

Hi,

httplock_form.inc line 34: Please use constant for roles, manipulation of roles sometimes changes the id.

if (!in_array("3", $selected_roles)) {
$selected_roles[] = "3";
}

httplock.module line 128: Using Constant will be a good idea, making a change single place will affect in other places.
if (in_array(3, array_keys($validated_user->roles))) {

httplock_form.inc line no 134: Merge the fields in a single line
$denied_node_ids = db_select('node', 'n')
->fields('n', array('nid'))
->fields('n', array('title'))
->fields('n', array('type'))

better to be like
->fields('n', array('nid', 'title', 'type'))

httplock_form.inc line no 168: What is the use of echo here?

httplock_form.inc line no 94: minor spelling mistake. "origonating"

Thanks

Hi, nice project! This is super useful. There are a few things though that I am wondering. 1) Can the config form default to admin only so that without any changes it will operate as most people will probably want it to operate. It seems like when in doubt granting less access is the way to go for something like this. 2) I'm concerned about the seemingly extraneous use of 'include_once "httplock.methods.inc";' in the httplock_form.inc file. I ran the module both with the line and without the line and there is seemingly no difference. Perhaps I'm missing something, but I think you only need to run it once from the.module file so long as the module is installed. Without the include in the .install file it won't uninstall since .install is all that's going to run on uninstall. But for the form file It seems like the statement might be redundant with the .module file. I could be mistaken about all of this and if so, I'm curious to learn more! 3) You should update the project application page so that the git clone is a clean generic clone code to http:// url without the user attribution.

I'm definitely excited to use this module. Thanks!

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

manual review:

1. please add the differences to the existing projects to your project page, so that users can make an educated decision which module to use.
2. Your code does not seem to include any automated tests? Please consider writing Simpletests: https://drupal.org/simpletest
3. httplock_preprocess_page(): why hook_preprocess_page()? I think hook_init() might be more appropriate? hook_preprocess_page() is only called if an actual page is displayed, does that mean there is an access bypass security vulnerability for any other menu routes that don't render pages?
4. httplock_test_user_and_role(): you cannot hard-code the administrative role here with "3", as that might be different on Drupal sites. You need to use variable_get('user_admin_role').
5. httplock_test_user_and_role(): you are calling user_authenticate without flood protection here, so an attacker can try to brute-force the password for a given user account. Please use the flood_*() functions to prevent that. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
6. httplock_test_whitelist(): there is an empty if clause in there, should that be removed?
7. httplock_ip_string_validate(): don't use "echo" here, is that a debugging leftover?

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.