Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

online tracking turned off vs online menu #3681

Closed
Jimmi08 opened this issue Feb 18, 2019 · 9 comments
Closed

online tracking turned off vs online menu #3681

Jimmi08 opened this issue Feb 18, 2019 · 9 comments
Assignees
Labels
type: bug A problem that should not be happening
Milestone

Comments

@Jimmi08
Copy link
Contributor

Jimmi08 commented Feb 18, 2019

If you turn online tracking off and forget to remove the online menu, the result is this:

image

There should be a similar message like with stats plugin at least for main admin that tracking is off and for others, this shouldn't be displayed at all.

@CaMer0n CaMer0n added the type: bug A problem that should not be happening label Feb 18, 2019
@Moc Moc self-assigned this Sep 23, 2019
@Moc
Copy link
Member

Moc commented Sep 23, 2019

The code for this is already there, but not working because e_TRACKING_DISABLED is not defined properly in the goOnline() method in online_class.php.

if(!defined('e_TRACKING_DISABLED'))
{
	$text = $tp->parseTemplate($ONLINE_TEMPLATE['enabled'], TRUE, $online_shortcodes);
}
else
{
	if (ADMIN)
	{
		$text = $tp->parseTemplate($ONLINE_TEMPLATE['disabled'], TRUE, $online_shortcodes);
	}
	else
	{
		return;
	}
}

Working on it.

@Moc
Copy link
Member

Moc commented Sep 23, 2019

@CaMer0n
It seems there are 3 issues:

  1. This check in class2.php is not triggered for me:
if(!isset($_E107['no_online']) && varset($pref['track_online']))
{
	e107::getOnline()->goOnline($pref['track_online'], $pref['flood_protect']);
}

It's because varset($pref['track_online']) returns false, because it is 0 when disabled

  1. Another issue is the flood_protect pref. It seems this pref is not used at all but instead:
    define("PRFLAN_35", "Enable flood protection?"); is used for the pref antiflood1.

I propose changing

if(!isset($_E107['no_online']) && varset($pref['track_online']))
{
	e107::getOnline()->goOnline($pref['track_online'], $pref['flood_protect']);
}

to

if(!isset($_E107['no_online']))
{
	e107::getOnline()->goOnline($pref['track_online'], $pref['antiflood1']);
}

or use the constant FLOODPROTECT which seems to be defined earlier.

  1. The last issue in online_class.php is this:
	if($online_tracking === false && $flood_control === false)
		{
			define('e_TRACKING_DISABLED', true);		// Used in forum, online menu
			...

This means that aside from changing the preference Enable online user tracking (track_online), one ALSO has to disable to flood protection. Is this really needed?

As these are pretty important defines and methods in class2.php I am not changing it myself...

@Moc Moc added this to the e107 2.2.2 milestone Sep 23, 2019
@Moc
Copy link
Member

Moc commented Nov 14, 2019

Looking at this commit d5711fd

This

if($online_tracking || $flood_control)

was replaced with:

if($online_tracking === false && $flood_control === false)

I'm guessing it should be || (OR) not && (AND)

@CaMer0n
Copy link
Member

CaMer0n commented May 21, 2020

@Moc I believe you're correct.

@Moc
Copy link
Member

Moc commented May 22, 2020

@CaMer0n @Deltik Do you foresee any negative effects when applying the 3 changes I listed above?

@Deltik
Copy link
Member

Deltik commented May 22, 2020

Looks good to me. I was curious about why there are two different variables for flood protection (flood_protect and antiflood1).

flood_protect has been a pref since before 02 October 2002 (release version 0.5.4), before any releases available on SourceForge.

antiflood1 was introduced on 08 July 2004 by loloirie, apparently as a replacement for flood_protect:

$ git show 19468aec78
commit 19468aec789ded31d397796f045803c0eb14509b (HEAD)
Author: loloirie <loloirie@8cb2f43a-d3f2-4d94-9ba2-05e386d4b0b9>
Date:   Thu Jul 8 15:55:16 2004 +0000

    Use prefs for antiflood (antiflood func) and autoban (online func) and cleaned for old flood prefs

diff --git a/e107/class2.php b/e107/class2.php
index 86aad3cab..ef4b816f1 100644
--- a/e107/class2.php
+++ b/e107/class2.php
@@ -342,8 +342,8 @@ if(e_QUERY == "logout"){
 ban();
 
 define("TIMEOFFSET", $pref['time_offset']);
-define("FLOODTIME", $pref['flood_time']);
-define("FLOODHITS", $pref['flood_hits']);
+//define("FLOODTIME", $pref['flood_time']);
+//define("FLOODHITS", $pref['flood_hits']);
 
 if(strstr(e_SELF, $ADMIN_DIRECTORY) && $pref['admintheme'] && !$_POST['sitetheme']){
         if(strstr(e_SELF, "menus.php")){
@@ -365,7 +365,8 @@ if(strstr(e_SELF, $ADMIN_DIRECTORY) && $pref['admintheme'] && !$_POST['sitetheme
 
 if($pref['anon_post'] ? define("ANON", TRUE) : define("ANON", FALSE));
 if(Empty($pref['newsposts']) ? define("ITEMVIEW", 15) : define("ITEMVIEW", $pref['newsposts']));
-if($pref['flood_protect']){  define(FLOODPROTECT, TRUE); define(FLOODTIMEOUT, $pref['flood_timeout']); }
+
+if($pref['$antiflood1']==1){  define(FLOODPROTECT, TRUE); define(FLOODTIMEOUT, $pref['antiflood_timeout']); }
 
 define ("HEADERF", e_THEME."templates/header".$layout.".php");
 define ("FOOTERF", e_THEME."templates/footer".$layout.".php");
@@ -884,7 +885,7 @@ function online(){
                 }
         }
 
-        if(ADMIN){$online_pagecount=1;}
+        if(ADMIN || $pref['autoban']!=1){$online_pagecount=1;}
         if($online_pagecount > $online_bancount && $online_ip !="127.0.0.1"){
                 $sql -> db_Insert("banlist", "'$ip', '0', 'Hit count exceeded ($online_pagecount requests within allotted time)' ");
                 exit;
@@ -955,7 +956,7 @@ class floodprotect{
                 # - scope                                        public
                 */
                 $sql = new db;
-                if(FLOODPROTECTION == TRUE){
+                if(FLOODPROTECT == TRUE){
                         $sql -> db_Select($table, "*", "ORDER BY ".$orderfield." DESC LIMIT 1", "no_where");
                         $row = $sql -> db_Fetch();
                         return ($row[$orderfield] > (time() - FLOODTIMEOUT) ? FALSE : TRUE);

Perhaps we should introduce a database migration that chooses one pref name and removes the other we don't want?

@Moc Moc closed this as completed in c92b6c5 May 23, 2020
@Moc
Copy link
Member

Moc commented May 23, 2020

Done. This could use some testing. I hope no negative affects arise when it comes to flood protection and automated banning of those triggering the flood protection.

The online menu menu now functions correctly.

@CaMer0n The only thing that needs to be done I guess is removing the flood_protect pref which was not used anymore, like you said @Deltik. Where do we add such a routine?

@Deltik
Copy link
Member

Deltik commented May 23, 2020

@Moc: In e107_admin/update_routines.php, most likely. Do a check for the existence of the pref and then remove it if it is present. I think perhaps like:

if (e107::getConfig()->has('flood_protect'))
{
        if ($just_check)
        {
                return update_needed("Unused pref for flood protection");
        }
        else
        {
                e107::getConfig()->remove('flood_protect');
        }
}

@Moc Moc reopened this May 24, 2020
@Moc Moc unassigned CaMer0n May 24, 2020
@Moc
Copy link
Member

Moc commented May 28, 2020

@Deltik @CaMer0n I noticed update_core_prefs() in the upgrade routines:

function update_core_prefs($type='')

If I read it correctly, it will check for missing core prefs. Perhaps we expand this function to also delete core prefs that exist in the table, but are no longer present in the install.xml file (get_default_prefs())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A problem that should not be happening
Projects
None yet
Development

No branches or pull requests

4 participants