Get started with PHP-Fusion

Start a New Thread

Users Participated

  • CrappoMan
    Post made: 1
  • Basti
    Post made: 2
  • Chan
    Post made: 4

  1. PHP-Fusion Support Forums
  2. Development & Design
  3. Roadmap

Bug in checkgroup()-function

[Development Team Access] All Roadmap items end up here for discussions.

6 Replies 5,136 Views Last Updated on 3 years ago

Basti


Moderator

#1

Posted 4 years ago

This is the function:
Code Gist: Download source  


// 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-fu...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

Posts: 1103

Joined: 09/04/2007

Chan


Super Admin

#2

Posted 4 years ago

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_tweetsion_tweet

Posts: 3454

Joined: 25/09/2007

Basti


Moderator

#3

Posted 4 years ago

Solution 1:
A "dirty" fix:
user_groups.php
Replace Code with this:
Code Gist: Download source  


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

Posts: 1103

Joined: 09/04/2007

Chan


Super Admin

#4

Posted 4 years ago

I will respond to this soon. There gotta be a good way.
Lead Developer of PHP-Fusion
Developer Tweet: https://twitter.com/phpfusion_tweetsion_tweet

Posts: 3454

Joined: 25/09/2007

Chan


Super Admin

#5

Posted 3 years ago

Proposed fix.

Code Gist: Download source  


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; }
    }

Posts: 3454

Joined: 25/09/2007

CrappoMan


Moderator

#6

Posted 3 years ago

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.

Posts: 6

Joined: 29/02/2004

Chan


Super Admin

#7

Posted 3 years ago

Skip auto increment value to 104 when it reaches 100. It would be most optimised solution.

Posts: 3454

Joined: 25/09/2007

Jump to Forum:
6 users are online
1 member and 5 guests

afoster