[Rt-devel] MAJOR BUG in group membership.

[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.

Are you talking about a race condition here? The first thing
User->Create does is check that ACL.


> 
> 2)
>     my $principal_id = $principal->Create(PrincipalType => 'User',
>                                 Disabled => $args{'Disabled'},
>                                 ObjectId => '0');
>     $principal->__Set(Field => 'ObjectId', Value => $principal_id);

> ??? IMHO it'll die if we couldn't create a principal Id.
> Or update all principals because $principal object don't have fetched 
> fields :)))))
> 
It'll set it for id 0 or id '', which won't find any.

>     # If we couldn't create a principal Id, get the fuck out.
>     unless ($principal_id) {
> ??? We check it too old.

Too late? yes. you're correct.

> Attached script fix tables, it _must_ be included/merged in RT update 
> script in next release.

I would like to hear of _any_ other site that has run into the issue
that ruslan has.

> 
> 			Good luck. Ruslan.
> 
> An angry PS:

...flame removed. RT is free software. If it breaks and you have a
support contract, we guarantee that we'll do everything in our power to
fix your issue as soon as possible.  If it breaks and you _don't_ have a
support contract, we'll still generally do our best to make sure that RT
gets fixed as soon as possible. But if you're not paying us to make sure
that RT works for you, heaping abuse on us is a really bad way to get us
to help you.  

A patch for RT/User_Overlay.pm is attached. Feedback would be
appreciated.

-- 
-------------- next part --------------
=== lib/RT/User_Overlay.pm
==================================================================
--- lib/RT/User_Overlay.pm  (revision 2144)
+++ lib/RT/User_Overlay.pm  (local)
@@ -263,14 +263,15 @@
     my $principal_id = $principal->Create(PrincipalType => 'User',
                                 Disabled => $args{'Disabled'},
                                 ObjectId => '0');
-    $principal->__Set(Field => 'ObjectId', Value => $principal_id);
     # If we couldn't create a principal Id, get the fuck out.
     unless ($principal_id) {
         $RT::Handle->Rollback();
-        $RT::Logger->crit("Couldn't create a Principal on new user create. Strange things are afoot at the circle K");
+        $RT::Logger->crit("Couldn't create a Principal on new user create.");
+        $RT::Logger->crit("Strange things are afoot at the circle K");
         return ( 0, $self->loc('Could not create user') );
     }
 
+    $principal->__Set(Field => 'ObjectId', Value => $principal_id);
     delete $args{'Disabled'};
 
     $self->SUPER::Create(id => $principal_id , %args);
@@ -284,15 +285,6 @@
         return ( 0, $self->loc('Could not create user') );
     }
 
-
-    #TODO post 2.0
-    #if ($args{'SendWelcomeMessage'}) {
-    #	#TODO: Check if the email exists and looks valid
-    #	#TODO: Send the user a "welcome message" 
-    #}
-
-
-
     my $aclstash = RT::Group->new($self->CurrentUser);
     my $stash_id = $aclstash->_CreateACLEquivalenceGroup($principal);
 
@@ -302,27 +294,50 @@
         return ( 0, $self->loc('Could not create user') );
     }
 
-    $RT::Handle->Commit;
 
-    #$RT::Logger->debug("Adding the user as a member of everyone"); 
     my $everyone = RT::Group->new($self->CurrentUser);
     $everyone->LoadSystemInternalGroup('Everyone');
-    $everyone->AddMember($self->PrincipalId);
+    unless ($everyone->id) {
+        $RT::Logger->crit("Could not load Everyone group on user creation.");
+        $RT::Handle->Rollback();
+        return ( 0, $self->loc('Could not create user') );
+    }
 
+
+    my ($everyone_id, $everyone_msg) = $everyone->AddMember($self->PrincipalId);
+    unless ($everyone_id) {
+        $RT::Logger->crit("Could not add user to Everyone group on user creation.");
+        $RT::Logger->crit($everyone_msg);
+        $RT::Handle->Rollback();
+        return ( 0, $self->loc('Could not create user') );
+    }
+
+
+    my $access_class = RT::Group->new($self->CurrentUser);
     if ($privileged)  {
-        my $priv = RT::Group->new($self->CurrentUser);
-        #$RT::Logger->debug("Making ".$self->Id." a privileged user");
-        $priv->LoadSystemInternalGroup('Privileged');
-        $priv->AddMember($self->PrincipalId);  
+        $access_class->LoadSystemInternalGroup('Privileged');
     } else {
-        my $unpriv = RT::Group->new($self->CurrentUser);
-        #$RT::Logger->debug("Making ".$self->Id." an unprivileged user");
-        $unpriv->LoadSystemInternalGroup('Unprivileged');
-        $unpriv->AddMember($self->PrincipalId);  
+        $access_class->LoadSystemInternalGroup('Unprivileged');
     }
 
+    unless ($access_class->id) {
+        $RT::Logger->crit("Could not load Privileged or Unprivileged group on user creation");
+        $RT::Handle->Rollback();
+        return ( 0, $self->loc('Could not create user') );
+    }
 
-   #  $RT::Logger->debug("Finished creating the user");
+
+    my ($ac_id, $ac_msg) = $access_class->AddMember($self->PrincipalId);  
+
+    unless ($ac_id) {
+        $RT::Logger->crit("Could not add user to Privileged or Unprivileged group on user creation. Aborted");
+        $RT::Logger->crit($ac_msg);
+        $RT::Handle->Rollback();
+        return ( 0, $self->loc('Could not create user') );
+    }
+
+
+    $RT::Handle->Commit;
     return ( $id, $self->loc('User created') );
 }