Fields should be filtered so developers can change them

[iandunn]

I've got a couple sites that I'd like to use this plugin on, but they both require visitors to register for accounts in order to access some of the sites' content. The accounts are just subscribers (and therefore don't have access to anything sensative), and the majority of the users are not technical, so I think that the hard-coded password complexity requirements are too strict, and will just end up frustrating users.

Ideally, it'd be great to be able to set the requirements based on the user role, but it'd also be easy to just filter the values in login_security_solution_admin::set_fields(), so that a developer can override them with what they prefer.

protected function set_fields() {
        $fields = array(
            'idle_timeout' => array(
                'group' => 'misc',
                'label' => __("Idle Timeout", self::ID),
                'text' => __("Close inactive sessions after this many minutes. 0 disables this feature.", self::ID),
                'type' => 'int',
            ),
            'disable_logins' => array(
                'group' => 'misc',
                'label' => __("Maintenance Mode", self::ID),
                'text' => __("Disable logins from users who are not administrators and disable posting of comments?", self::ID),
                'type' => 'bool',
                'bool0' => __("Off, let all users log in.", self::ID),
                'bool1' => __("On, disable comments and only let administrators log in.", self::ID),
            ),
            'deactivate_deletes_data' => array(
                'group' => 'misc',
                'label' => __("Deactivation", self::ID),
                'text' => __("Should deactivating the plugin remove all of the plugin's data and settings?", self::ID),
                'type' => 'bool',
                'bool0' => __("No, preserve the data for future use.", self::ID),
                'bool1' => __("Yes, delete the damn data.", self::ID),
            ),

            'login_fail_minutes' => array(
                'group' => 'login',
                'label' => __("Match Time", self::ID),
                'text' => __("How far back, in minutes, should login failures look for matching data?", self::ID),
                'type' => 'int',
            ),
            'login_fail_tier_2' => array(
                'group' => 'login',
                'label' => __("Delay Tier 2", self::ID),
                'text' => sprintf(__("How many matching login failures should it take to get into this (%d - %d second) Delay Tier? Must be >= %d.", self::ID), 4, 30, 2),
                'type' => 'int',
                'greater_than' => 2,
            ),
            'login_fail_tier_3' => array(
                'group' => 'login',
                'label' => __("Delay Tier 3", self::ID),
                'text' => sprintf(__("How many matching login failures should it take to get into this (%d - %d second) Delay Tier? Must be > Delay Tier 2.", self::ID), 25, 60),
                'type' => 'int',
            ),
            'admin_email' => array(
                'group' => 'login',
                'label' => __("Notifications To", self::ID),
                'text' => __("The email address(es) the failure and breach notifications should be sent to. For multiple addresses, separate them with commas. WordPress' 'admin_email' setting is used if none is provided here.", self::ID),
                'type' => 'string',
            ),
            'login_fail_notify' => array(
                'group' => 'login',
                'label' => __("Failure Notification", self::ID),
                'text' => __("Notify the administrator upon every x matching login failures. 0 disables this feature.", self::ID),
                'type' => 'int',
            ),
            'login_fail_breach_notify' => array(
                'group' => 'login',
                'label' => __("Breach Notification", self::ID),
                'text' => __("Notify the administrator if a successful login uses data matching x login failures. 0 disables this feature.", self::ID),
                'type' => 'int',
            ),
            'login_fail_breach_pw_force_change' => array(
                'group' => 'login',
                'label' => __("Breach Email Confirm", self::ID),
                'text' => __("If a successful login uses data matching x login failures, immediately log the user out and require them to use WordPress' lost password process. 0 disables this feature.", self::ID),
                'type' => 'int',
            ),

            'pw_length' => array(
                'group' => 'pw',
                'label' => __("Length", self::ID),
                'text' => sprintf(__("How long must passwords be? Must be >= %d.", self::ID), 8),
                'type' => 'int',
                'greater_than' => 8,
            ),
            'pw_complexity_exemption_length' => array(
                'group' => 'pw',
                'label' => __("Complexity Exemption", self::ID),
                'text' => sprintf(__("How long must passwords be to be exempt from the complexity requirements? Must be >= %d.", self::ID), 20),
                'type' => 'int',
                'greater_than' => 20,
            ),
            'pw_change_days' => array(
                'group' => 'pw',
                'label' => __("Aging", self::ID),
                'text' => __("How many days old can a password be before requiring it be changed? Not recommended. 0 disables this feature.", self::ID),
                'type' => 'int',
            ),
            'pw_change_grace_period_minutes' => array(
                'group' => 'pw',
                'label' => __("Grace Period", self::ID),
                'text' => sprintf(__("How many minutes should a user have to change their password once they know it has expired? Must be >= %d.", self::ID), 5),
                'type' => 'int',
                'greater_than' => 5,
            ),
            'pw_reuse_count' => array(
                'group' => 'pw',
                'label' => __("History", self::ID),
                'text' => __("How many passwords should be remembered? Prevents reuse of old passwords. 0 disables this feature.", self::ID),
                'type' => 'int',
            ),
        );

        $this->fields = apply_filters( self::ID . 'set_fields', $fields );
    }

[convissor]

Hi Ian:

Thanks for putting some effort into making this plugin better. A few thoughts and questions.

How does adding/editing/removing fields on the settings page help with a goal such as modifying permission levels?

Allowing the settings page to be modified means the plugin can be modified from anywhere. This makes it harder to maintain because there can be an infinite number of behaviors.

Most importantly, providing admins the ability to modify which roles need strong passwords will lead to some admins letting authors and contributors, for example, use weak passwords. Such admins will mistakenly think that since such users can't modify settings, they can't pose problems. But I would give consideration to a patch that just lets subscribers slack off on the password requirements.

Thanks,

--Dan

[iandunn]

I didn't dig too far into the code, so I guess there would also need to be filters on the sections of code that enforce the settings.

Adding hooks to a plugin does allow changes to be made from outside the plugin, but that's the whole point. Think about what WordPress itself would be like it you couldn't hook into it and customize the behavior from outside via a plugin. It'd be a tiny fraction of what it is today. The success of WordPress is, in large part, due to how extensible it is. That does allow for more points of failure, but that's a small price to pay, and the developer making the modifications assumes the responsibility for that.

I think that those same principles apply to plugins as well. All plugins should have hooks throughout them so that other developers can extend/customize them without having to fork them. When I talk to other developers, there seems to be a consensus for that, too. Here's a few examples:

• http://picklewagon.com/2011/09/26/wordpress-plugins-as-frameworks/
• http://willnorris.com/2009/06/wordpress-plugin-pet-peeve-3-not-being-extensible
• http://wordpress.tv/2010/12/03/brandon-dove-the-pluggable-plugin/ as a few examples.

You can never anticipate how people will use your plugin, and I think it's wrong to assume that one size fits all, or to force your opinions on how a plugin should be used on users. If you do that, you're placing artificial restrictions on the user and taking control away from them. It makes perfect sense to start with secure default settings, and to warn the user if they're about to do something you consider risky, but you should never force them to do it your way, especially when you don't know their circumstances.

I think this still applies for security, because every site has different needs, and security is always a tradeoff between risk and convenience. Does a site on a small business' intranet need the same level of security as their public-facing site? Does a mom-and-pop brochure site need the same level of security as an eCommerce application for a Fortune 500? Does a prototype that's going to be thrown away in a week need strict and inconvenient policies?

Login Security Solution is by far the best plugin out there for protecting against login attacks, but there are several sites where I just can't use it because the policies are too strict and I can't change them. The clients just won't accept it. If these sites were highly visible or stored confidential information, then I'd tell the clients that they had to have strict policies, but it's just not appropriate for some of these circumstances. Users will just end up writing their passwords down on sticky notes next to their monitors, or give up and not use the site at all.

That's my opinion, anyway. Let me know what you think. I'm happy to write a patch if we can come to a consensus on what it should look like.

[LinuxBozo]

@iandunn has a valid point, one that I deal with in the US Government consulting sector. Each agency has their own password policy based on most of the rules that are enforced by this plugin. However, the one size fits all enforcement, while above and beyond what each agency requires, and is good, is not suitable for them. They want ONLY the requirements they specify. No more, no less. Even if it is more secure. They are only looking to "check the boxes" as it were when implementing their IT systems. Most agencies base their policies on NIST 800-53, which specifies for this particular matter:

a. Enforces minimum password complexity of [Assignment: organization-defined requirements for case sensitivity, number of characters, mix of upper-case letters, lower-case letters, numbers, and special characters, including minimum requirements for each type];

So what happens is that Agency A specifies the password requirements as having to contain at last 2 upper case, 2 lower case, and 4 numbers being at least 8 characters long. Agency B then says that you have to have at least 1 each of upper, lower, number and special, and it has to be a min of 16 chars long. You can see how the plugin would be better served by being able to specify the policy, rather than dictating it explicitly and making it immutable.

BTW, I've already submitted a couple pull requests related to other NIST 800-53 controls, so I may end up tackling this at some point as well to provide that fine grained control of the password policy.