Oh no! Where's the JavaScript?
Your Web browser does not have JavaScript enabled or does not support JavaScript. Please enable JavaScript on your Web browser to properly view this Web site, or upgrade to a Web browser that does support JavaScript.
Welcome to the Official Site of PHP-Fusion
Sign In
Not a member yet? Click here to register.
Navigation
6 Replies | Latest reply on 03-01-2017 04:08 by Chan
Basti
Basti Posted 4 years ago 1103 posts

Bug in checkgroup()-function

This is the function:

// Check if user is assigned to the specified user group
function checkgroup($group) {
   if (iSUPERADMIN) { return true; }
   elseif (iADMIN && ($group == "0" || $group == "101" || $group == "102")) { return true;
   } elseif (iMEMBER && ($group == "0" || $group == "101")) { return true;
   } elseif (iGUEST && $group == "0") { return true;
   } elseif (iMEMBER && $group && in_array($group, explode(".", iUSER_GROUPS))) {
      return true;
   } else {
      return false;
   }
}


Let's say we habe a group "Support Team".
Like this one: http://www.php-fusion.co.uk/profile.p...roup_id=23

the ID of this group is 23.
checkgroup("23") returns true if a site user is part of this group.

everything is ok so far.

Now here is the bug:

Let's say we have a group "Development" with ID "101".
And we have a User XXX who is member on your site, but not a member of the Group.

the function is: checkgroup("101")

and now the function returns true ! But the User XXX is not a part of this group!


Notice: This bug only appears when the ID of the group is 101 or 102.

Go to roadmap item #1460

[PHP-Fusion Crew Member & Admin from June 2008 - December 2010]

http://basti2web.de - Support Site for my infusions

6 Replies

Sort replies by
Chan
ChanPosted 4 years ago 3478 posts
Great. that's userlevel not usergroup, yeah what you meantioned is correct. I noticed this issue as well.

Solution 1:
What we can do is to skip 101 and 102 during insert of new group. When we do setup, have 101 and 102 created and make it as reserved so it automatically skips it.

Solution 2:
Recode getusergroup.
Lead Developer of PHP-Fusion
Developer Tweet: https://twitter.com/phpfusion_tweet
Basti
BastiPosted 4 years ago 1103 posts
Solution 1:
A "dirty" fix:
user_groups.php
Replace Code with this:

if (isset($_POST['save_group'])) {
   $group_name = stripinput($_POST['group_name']);
   $group_description = stripinput($_POST['group_description']);
   if ($group_name) {
      if (isset($_GET['group_id']) && isnum($_GET['group_id'])) {
         $result = dbquery("UPDATE ".DB_USER_GROUPS." SET group_name='$group_name', group_description='$group_description' WHERE group_id='".$_GET['group_id']."'");
         redirect(FUSION_SELF.$aidlink."&status=su");
      } else {
         $result = dbquery("INSERT INTO ".DB_USER_GROUPS." (group_name, group_description) VALUES ('$group_name', '$group_description')");
         // Fix:
         $id = mysql_insert_id();
         if($id == 101) {
            $result = dbquery("INSERT INTO ".DB_USER_GROUPS." (group_name, group_description) VALUES ('dummy', 'dummy')");
            $result = dbquery("DELETE FROM ".DB_USER_GROUPS." WHERE group_id='101' OR group_id='102'");
            $result = dbquery("INSERT INTO ".DB_USER_GROUPS." (group_name, group_description) VALUES ('$group_name', '$group_description')");
         }
         // end of fix
         redirect(FUSION_SELF.$aidlink."&status=sn");
      }
   } else {
      redirect(FUSION_SELF.$aidlink);
   }
}


Solution 2:

+ Would be a more clean solution.
- But needs more code rewriting.
- No compatibility to old infusions, which use this function?
[PHP-Fusion Crew Member & Admin from June 2008 - December 2010]

http://basti2web.de - Support Site for my infusions
Chan
ChanPosted 4 years ago 3478 posts
I will respond to this soon. There gotta be a good way.
Lead Developer of PHP-Fusion
Developer Tweet: https://twitter.com/phpfusion_tweet
Chan
ChanPosted 4 years ago 3478 posts
Proposed fix.


function checkgroup($group) {
        // previous checkgroup(101) and checkgroup(102) is redundant. - use IMEMBER and IADMIN respectively.

        if (iSUPERADMIN) {
            return true;
        } elseif (iMEMBER && $group && in_array($group, explode('.', iUSER_GROUPS))) {
            return true;
        } else { return false; }
    }
CrappoMan
CrappoManPosted 4 years ago 6 posts
When this function was originally introduced along with the "xxx_access" db fields, it wasn't envisaged that there would be a need for more than 100 user groups, so 101-103 was chosen for builtin groups (member, admin, etc).

A few functions are affected by this, e.g: getgroupname() will return the builtin group name, not the actual user group name, for any "group_id" between 101-103

In hindsight, it would have been better for setup.php to populate the usergroups table with the default user levels starting at 0, rather than having them hardcoded.

A better system definitely needs to be introduced, but the 101-103 values are used in a few places throughout the codebase.
Chan
ChanPosted 4 years ago 3478 posts
Skip auto increment value to 104 when it reaches 100. It would be most optimised solution.

Actions

Sharing

More topics like this

You can view all discussion threads in this forum.
You can start a new discussion thread in this forum.
You cannot reply in this discussion thread.
You cannot start on a poll in this forum.
You cannot upload attachments in this forum.
You cannot download attachments in this forum.
Moderator: Development Team
Basti

Bug in checkgroup()-function
by Basti Posted 4 years ago