[Rt-devel] MAJOR BUG in group membership.

[Jesse Vincent]

On Sat, Apr 24, 2004 at 02:15:38PM +0400, Ruslan U. Zakirov wrote:
> Jesse Vincent пишет:
> >
> >
> >On Fri, Apr 23, 2004 at 10:20:33PM +0400, Ruslan U. Zakirov wrote:
> >
> >>Bug located in $UserObj->Create().
> >
> >
> >And fixed, I believe. Please test out the attached patch
> >
> >
> >>This function buggy in many ways like an 'Maasdam' cheese.
> >>1) Doesn't check return values when add users to system groups. 
> >>AddMember check ACLs. Report below. Step to reproduce: Grant user with 
> >>'AdminUsers', revoke 'AdminGroup' and use his account to create new 
> >>user. :) Not all work under SU.

> s/AdminGroup/AdminGroupMembership/

Ok. That makes somewhat more sense. An additional patch is attached.
 
> I think 'AdminUsers' right is required and sufficient right to create 
> new user. Don't you think so?

Yes. Ditto with "SetPrivileged"

> Shame on me. I'm so sorry. Bad days, no rest. I did forget about all 
> this in OpenSource, thanks for remember me.

No worries. We all get frustrated by software sometimes. I'm betting
that RT still hasn't frustrated you nearly as much as it as frustrated
me ;) 


> Jesse, you are using CurrentUser account to manage memebership in system 
> internal groups, don't you think that this is a bit of misslogic. As I 
> understand you can't use $RT::SystemUser account, because it can be 
> unavailable right now. Yes?

Nope. to create RT::SystemUser, we use _BootstrapCreate. But instead of
using the SystemUser, we can use the acl-less versions of AddMember and
DeleteMember, _AddMember and _DeleteMember.

-- 
-------------- next part --------------
=== lib/RT/User_Overlay.pm
==================================================================
--- lib/RT/User_Overlay.pm  (revision 2148)
+++ lib/RT/User_Overlay.pm  (local)
@@ -304,7 +304,7 @@
     }
 
 
-    my ($everyone_id, $everyone_msg) = $everyone->AddMember($self->PrincipalId);
+    my ($everyone_id, $everyone_msg) = $everyone->_AddMember( InsideTransaction => 1, PrincipalId => $self->PrincipalId);
     unless ($everyone_id) {
         $RT::Logger->crit("Could not add user to Everyone group on user creation.");
         $RT::Logger->crit($everyone_msg);
@@ -327,7 +327,7 @@
     }
 
 
-    my ($ac_id, $ac_msg) = $access_class->AddMember($self->PrincipalId);  
+    my ($ac_id, $ac_msg) = $access_class->_AddMember( InsideTransaction => 1, PrincipalId => $self->PrincipalId);  
 
     unless ($ac_id) {
         $RT::Logger->crit("Could not add user to Privileged or Unprivileged group on user creation. Aborted");
@@ -375,6 +375,10 @@
     my $self = shift;
     my $val = shift;
 
+    #Check the ACL
+    unless ( $self->CurrentUser->HasRight(Right => 'AdminUsers', Object => $RT::System) ) {
+        return ( 0, $self->loc('Permission Denied') );
+    }
     my $priv = RT::Group->new($self->CurrentUser);
     $priv->LoadSystemInternalGroup('Privileged');
    
@@ -396,7 +400,7 @@
             return (0,$self->loc("That user is already privileged"));
         }
         if ($unpriv->HasMember($self->PrincipalObj)) {
-            $unpriv->DeleteMember($self->PrincipalId);
+            $unpriv->_DeleteMember($self->PrincipalId);
         } else {
         # if we had layered transactions, life would be good
         # sadly, we have to just go ahead, even if something
@@ -404,7 +408,7 @@
             $RT::Logger->crit("User ".$self->Id." is neither privileged nor ".
                 "unprivileged. something is drastically wrong.");
         }
-        my ($status, $msg) = $priv->AddMember($self->PrincipalId);  
+        my ($status, $msg) = $priv->_AddMember( InsideTransaction => 1, PrincipalId => $self->PrincipalId);  
         if ($status) {
             return (1, $self->loc("That user is now privileged"));
         } else {
@@ -417,7 +421,7 @@
             return (0,$self->loc("That user is already unprivileged"));
         }
         if ($priv->HasMember($self->PrincipalObj)) {
-            $priv->DeleteMember($self->PrincipalId);
+            $priv->_DeleteMember( $self->PrincipalId);
         } else {
         # if we had layered transactions, life would be good
         # sadly, we have to just go ahead, even if something
@@ -425,7 +429,7 @@
             $RT::Logger->crit("User ".$self->Id." is neither privileged nor ".
                 "unprivileged. something is drastically wrong.");
         }
-        my ($status, $msg) = $unpriv->AddMember($self->PrincipalId);  
+        my ($status, $msg) = $unpriv->_AddMember( InsideTransaction => 1, PrincipalId => $self->PrincipalId);  
         if ($status) {
             return (1, $self->loc("That user is now unprivileged"));
         } else {