This module enabled you to modify the behavior of the user_login form by modifying the core flood control for user logins, disabling autocomplete on the form, and turning on whitelisting of IP addresses.

Because lists of IP addresses often become confusing, there's support for simple comments in the whitelist field. See below for example:
Whitelist example image

All features can be enabled separately and the installation is meant to be non-intrusive (i.e. no site behavior is changed upon installation).

Project page: https://drupal.org/sandbox/Sleavely/2045595
git clone --branch 7.x-1.x Sleavely@git.drupal.org:sandbox/Sleavely/2045595.git

Comments

paul rowell’s picture

paul rowell commented
Status: Active » Needs work

Hi Sleavely.

Ventral.org is throwing up a few things http://ventral.org/pareview/httpgitdrupalorgsandboxsleavely2045595git (too long to post here really)

You should also run Coder through your module as well: https://drupal.org/project/coder

The module should also have a ReadMe.txt file

Once the above is done I'll have a real nosey into the functionality. Thanks :)

Sleavely’s picture

Sleavely commented

How strict are the Drupal Coding Standards; i.e. is the indentation a dealbreaker? I'm currently adhering to coding guidelines for the project which I originally wrote the module, and maintaining it with two parallel styleguides is going to be awkward.

As for the first block of warnings regarding user_failed_login_ip_limit and user_failed_login_ip_window they are used by user.module:2140 in Drupal 7.22 core, and are declared long before the login_security module is activated.

I'll admit that there's surely room for improvement (when isn't there? :)), but I truthfully don't believe that it's in any way hard to read the code as long as you have syntax highlighting. I wouldn't exactly call it a rocket science project like Workbench or any of the other cool guys. ;)

paul rowell’s picture

paul rowell commented

Hey Sleavely,

The coding standards - https://drupal.org/coding-standards - "All new code should follow the current standards" say 'should' rather than 'must' but saying that I've not seen a module pass without it. I'm not 100% certain if it'll be a blocker so you'll need to ask someone who has been here a while. However I'm not sure why a drupal project would have different coding standards to Drupal itself, but I'm sure there must be solid reasons. This would also count for https://drupal.org/node/1354 - being a Drupal module, Drupal comment standards *should* be used.

As for the rest:

ReadMe.txt

Still appears to be missing.

login_security.info

Small thing - adding a configure will place the configure link in the module list page, allowing users to find the admin page easier.
configure = admin/config/people/login-security

login_security.install

Function comments might be needed here i.e :

/**
 * Implements hook_install().
 */

login_security.module

Functionality wise it looks good, I can see any obvious coding errors - Just the issue with conflicts between your project coding/commenting standards and Drupals - perhaps you could shed some more light on this?

Thanks

vwX’s picture

vwX commented

I installed and tested this module. Like the functionality, have some sites I would like to use it on. I agree with Paul that you should follow the coding standards.

Sleavely’s picture

Sleavely commented
Status: Needs work » Active

Hi fellas,

I've added a README.txt and tried to adhere to the coding standards; so pareview doesn't seem too unhappy with it anymore.

Is there anything else that strikes you as missing?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Active » Needs review

Looks like this needs review? See https://drupal.org/node/532400

PA robot’s picture

PA robot commented

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.

vwX’s picture

vwX commented
Status: Needs review » Reviewed & tested by the community

I reviewed and module works, code looks fine to me and I'm using it on a site.

One thing, maybe this would be an issue. Currently the core user module sets the watchdog severity to warning if, for example, someone tries to register on a site where registration is blocked. Maybe the watchdog alert in this should module should also be at that level.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Reviewed & tested by the community » Needs work
Master Branch
It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
  • You don't need to set all the default variables in login_security_install(), just provide the correct default when you read the value with variable_get().
  • Why do you need login_security_update_7001() - is that leftover from developing the module?

Otherwise looks pretty nice and useful.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Sleavely’s picture

Sleavely commented

@vwX:
Huh, that's odd. I'd say that qualifies more as a notice since it is expected behavior. From the API page:

WATCHDOG_NOTICE: (default) Normal but significant conditions.

Perhaps it is the user module that needs to be looked at. :-P

@kscheirer:
Yeah, for the initial release I've been using the master branch because everything is part of the same feature (release). Sorry 'bout that!

I set the variables in hook_install() because it is convenient to know that the variables are set and that I don't have to repeat the default values everywhere.

Yes, login_security_update_7001() is a leftover.

Sleavely’s picture

Sleavely commented

Oh, I've now read Release naming conventions and understand that my using the master branch is not the Drupal way. I've created a dev branch and a release candidate tag.

Sleavely’s picture

Sleavely commented
Status: Needs work » Needs review
kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Needs review » Needs work

Thanks, can you remove the master branch and login_security_update_7001() altogether?

----
Top Shelf Modules - Crafted, Curated, Contributed.

Sleavely’s picture

Sleavely commented
Status: Needs work » Needs review

Done!

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Needs review » Needs work

Thanks! Can you remove the 7.x-1.0-rc2 branch as well, and just leave 7.x-1.x?

----
Top Shelf Modules - Crafted, Curated, Contributed.

Sleavely’s picture

Sleavely commented
Status: Needs work » Needs review

Now I'm starting to feel like you're just pushing me around ;-)

I've removed the branch, but I'm curious; per definition a Release Candidate is a piece of code that is ready to be tagged as a stable version unless dealbreaking issues emerge - that's the stage I feel like we're in here. Did I miss something?

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Needs review » Reviewed & tested by the community

Sorry Sleavely, that did come off as kinda bossy. Documentation about release naming conventions and creating a branch in git should explain a little more.

Basically the branch name should be 7.x-1.x, and then when you're ready to create a release, you use a tag. The drupal packaging script will see that tag and create the actual release for you.

----
Top Shelf Modules - Crafted, Curated, Contributed.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Issue summary: View changes

Changed which branch to checkout

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

It's been a month without any problems reported, so I'm promoting this myself as per https://drupal.org/node/1125818.

Thanks for your contribution, Sleavely!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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.

Thanks to the dedicated reviewer(s) as well.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.