Security page feedback and improvements

The page that is actually linked to from within CP is https://link.classicpress.net/forum/security. This is a redirect and I’ve pointed it at the Core Development forum for now, so the link is effectively fixed even though the URL you pasted will remain dead. Since the Security forum no longer exists we will also need to update the wording in CP.

I am betting that those same >90% of sites have zero entries in the ClassicPress security menu, so with the addition of a new option that allows users to hide it, this issue would be mostly solved.

This is something we can do today. The other option that makes sense to me would be to just remove the security page completely, but that would need to wait until a new major version. It’s not as clear to me how to move the ClassicPress Security menu under Settings without breaking it, given that it expects to have sub-items when plugins register them.

2 Likes

Well, “solved” until they enable it.

Users would have that choice.

I agree with the concerns raised here, and I am looking for the best solution I can in the shortest amount of time possible. That means it’s not going to be perfect.

I started a PR: Security page improvements by nylen · Pull Request #779 · ClassicPress/ClassicPress · GitHub

2 Likes

Yeah, I know it won’t be perfect right off the bat; just noting that “solved” wasn’t really accurate there.

What if we rename it so it’s not a generic Security tab? Maybe Security Center, Security Settings, Security Options.

To go back to my previous suggestion, instead of creating sub-items in the menu plugins should create “cards” that link to their respective settings pages. As far as I can tell, that’s the only other option unless we introduce a separate menu on the Security page.

One thing to consider, if we agree that Security page should be sub-item of Settings menu then we should follow through with that and make top-level Security menu with sub-items disabled unless sub-items are added to keep it backwards compatible and not break any implementations. I doubt we would break anything but there might be a couple that use it.

I would go with CP Security to make it a bit clearer that it’s part of CP itself.

This is another option, but I don’t think it’s a good idea to make a menu item magically jump around depending on what plugins are installed on a site, this would also be unexpected and confusing behavior. “You might find Security under Settings… unless you have certain plugins installed… but it’s not easy to know which plugins those are…”

I would rather see it hidden in the v1 release series (based on a user decision, so that there are no back compat concerns) and removed in the v2 release series.

2 Likes

I think that’s a good way to proceed. For the name, what if we borrow from Site Health and call it Site Security?

I’m OK with CP Security if others like it, but would prefer something a bit more descriptive.

2 Likes

Good to see this is being discussed. I never understood the point of the extra Security menu item and ended up putting in some code to hide it as it seemed useless.

To support @viktor, CP Security suggests to me a specific security plugin developed by CP (like the SEO and Commerce options). Site Security sounds more generic.

1 Like

Here’s the mu-plugin I use to remove the Security stuff. It’s based on Code Potent’s gist but simply removes the Security icon instead of replacing it with text.

<?php
function zp_remove_security_menu() {
	remove_menu_page('security.php');
}
add_action('admin_menu', 'zp_remove_security_menu');

function zp_remove_security_icon() {
	if (!function_exists('\add_security_page')) {
		return;
	}
	foreach (array_keys(get_plugins()) as $plugin) {
		$func = '_security_page_action_links';
		$hook = 'plugin_action_links_'.$plugin;
		if (has_filter($hook, $func)) {
			remove_filter($hook, $func, PHP_INT_MAX);
		}
	}
}
add_action('load-plugins.php', 'zp_remove_security_icon', PHP_INT_MAX);

Just save this as remove-cp-security.php or any name you like, and drop it in the mu-plugins folder.

Just a few comments here on this topic:

  1. I wouldn’t prefix anything with “CP”, such as “CP Security” to make it clear that it’s native to CP. Users aren’t thick and they don’t need to be reminded that a feature is core to CP. That’s counter-intuitive. It’s actually the other way about. Have a look around WordPress’ admin to see anything prefixed with WP - the only real examples I found was plugins/themes use the “WP” prefix.

  2. We have a security plugin and frankly it’s impractical to use the Security page when we have so many pages in our plugin itself.
    WP has actually created quite a useful sub-tool through Site Health which does a great job in encompassing many different elements of the site, including security. This Site Health tool has been mentioned in this thread but I get the sense that if it’s in “WP Core”, then you don’t want it in “CP Core”. That’s also counter-intuitive. The ideal approach is to take the best parts and ideas of what’s out there and incorporate where you can. Copy if you have to.

  3. Whichever approach is taken, some serious thought needs to be given to the practicalities of implementation for a new feature both from UX and developer standpoints. Was there any in-depth digging into what the Security page would and should be? From some of the earlier comments on this page, it sounds like it was partly a user-voted project and a way to help distinguish CP from WP.

Simply moving it to a submenu, or changing its name, or whatever bandaid doesn’t solve the underlying problem. My suggestion would be to removing it entirely and bringing it back in another form once it’s been thought over.

It can be done at any time by removing the top level page for everyone by default (i.e. it’s empty/unused), and where someone, or a developer, has actually used it - which appears to be a small minority based on this discussion - only then it gets rendered.

I actually think having a cental security hub/page is a good idea, with a few CP Core settings supplied with a developer api to extend it. This is probably best as part of a “Site Health” -equivalent section. If you name it “Site Security” then it’s all about “Security”. If you name it “Site Health”, then it can be adopted for many different applications.

  1. If you make this a “Core Plugin”, then you’ll restrict developers’ ability to integrate since no-one is going to write an integration with something that may or may not be available on most sites. Unless you have a dependency management system. Does/Will CP have a dependency management system?

  2. I wouldn’t provide an “option” to enable/diable the menu. After developing a massive security plugin for a few years, one thing I know is this: users don’t need any more options. If you’re going to make it optional, make it a filter.

Just a few random thoughts.

6 Likes

Thanks for sharing your feedback, Paul. It’s good to get feedback from a developer of a security plugin.

That was discussed and I believe it’s in the plans, but no concrete plans on when it might be implemented. @james might be able to answer this question.

This is all started with this petition and as implementation began it stopped short of implementing additional features because the developer doing the work left.

I think combining the security page with the Site Health tool/page would be a good idea. There is a petition for Site Health.

I actually found a way to remove this menu, without breaking backwards compatibility, without requiring Developers to do any further work than the usual function needed to add their submenus in that menu…

Pushed change to Git here:

This is perhaps not perfect, but it removes that stain which the Security page is.
Once we decide that we have time, commitment and actual ideas to put in there, we can simple as delete a line of code, add it back.
Plugins which use the page are not going to break.

I hope we can see that this is a first step towards a CMS that can be taken serious. This page really is a joke and each time I propose CP to a client I hope to god they will not notice it.
It is nice if there is something inside.
But the current state (vanilla core) is just really not useful.
If a plugin manages(d) to add useful settings there, then that is cool and it still will work just as usual.

Yes, it “magically” adds/removes the menu page. But … I prefer a tad magic than a fist in the face of “looks cool but does nothing”

2 Likes

@Simone found an issue with this, which is that if a plugin loads the security page add_security_page on admin_menu hook, it will not show the menu.
The add_security_page doc unfortunately does not state when to load this. When you load it in init, which is typically used by plugins to initialize, the change I made will work.

So now we have a situation where we need to decide what is even expected to work. Let’s not have another “because it broke a plugin we do not change”.
The right thing IMO is to state in an api when it is available and when it is too late. I think init is not a bad hook for something like this.

Hooks that will work are
init, plugins_loaded, wp_loaded, for example.

@timkaye - you are the other dev I know using this function.
When do you hook it?

I hook it to admin_menu. But don’t worry about me; I’m happy to attach my function to the settings menu instead anyway. (I have it coded so it will work on WP this way; I can just remove the conditional that checks for the CP menu item.)

1 Like

So it seems to have been the common approach for some reason to use admin_menu

I personally would just ditch the page, but there have been concerns about the semver breaking if we do just remove something that existed as an API… and even if no one would care, this is actually true.

So I wanted to find a solution that allows us to safely remove this, without actually breaking anything and keeping even the functionality for those who want it.

Seems my plan is not going to work out, unless we would accept that since the DOC never specified the hook to add it… we could update the doc and say the plugins need to update the code.
It would not break the current installs, so they could prepare for it before CP even ships the change!

admin_menu is the correct hook because it’s on the admin menu! But I’d be OK with ditching the whole page; I don’t think anyone else uses it. (We did actually ask that some time ago, and no-one else responded.)

1 Like

If that is actually all that is needed, I have would have a solution for that.
I would however mean we expect the function to be loaded on admin_menu and otherwise it is doing it wrong.

If we can agree on that we should update the DOC and … the change I made will work, I just need to unset based on the submenu presence, instead of the menu’s presence.

It makes sense the admin_menu is the right hook because if we use init the sequence of menus are actually inverted (putting the plugin menu above the CP core one in the submenus), and the $submenu global becomes the plugin menu as keynote, which is also wrong

I pushed the change to GIT, it should now eliminate the menu correctly, as long the plugin uses admin_menu hook for this function, which would be the right thing to do if I get it right.
We also should update the doc, in any case, right?

2 Likes

That all sounds good to me.

1 Like

The DOC is updated, and @MattyRob already put some Unit Tests in place, so I guesshope we can bring this in next release…

This should probably have been a Petition, but then, nothing breaks or changes…

Case closed.

3 Likes