[Rt-devel] MAJOR BUG in group membership.

[Ruslan U. Zakirov]

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/

> 
> 
> Are you talking about a race condition here? The first thing
> User->Create does is check that ACL.
No you didn't understand a little. No races.
Rights checks aren't transparant. I can't imagine that user must have 
'AdminGroupMembership' to create new user, but $SysGroupObj->AddMember 
can. It's main problem for me and patch don't fix this small. Patch 
report 'Permission denied', but RT should create users even if current 
user has 'AdminUsers' right only.

I think 'AdminUsers' right is required and sufficient right to create 
new user. Don't you think so?
> 
> 
> 
>>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.
May be.
> 
> 
>>    # 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.
Yes, too late.

> 
> 
>>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.
Shame on me. I'm so sorry. Bad days, no rest. I did forget about all 
this in OpenSource, thanks for remember me.

> 
> A patch for RT/User_Overlay.pm is attached. Feedback would be
> appreciated.
I like patch, now RT does as much as possible to be 'fail save' on user 
creations. I wrote comments allready and also see below.

> 
> 
> 
> ------------------------------------------------------------------------
> 
> === 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);
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?

If I'm right then you have to implement special case into AddMember sub 
when target group is System then user require only 'AdminUsers' right.

>      $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') );
>  }
>