MAJOR BUG in group membership

Ruslan_U_Zakirov · April 22, 2004, 7:02pm

We have 449 user who has no member ship in group Everyone!

select count(1) from Principals pr left join GroupMembers gm on
(gm.MemberId = pr.id and gm.GroupId = 3) where pr.PrincipalType = ‘User’
and gm.id is null;

Most of this users were created via CLI.

We have
select count(1) from Principals where PrincipalType = ‘User’;
| count(1) |
| 43696 |
users.

Users by system groups
select g.id, g.Domain, g.Type, count(gm.id) members from GroupMembers
gm, Groups g where gm.GroupId in(3,4,5) and g.id = gm.GroupId group by
gm.GroupId;
| id | Domain | Type | members |
| 3 | SystemInternal | Everyone | 43247 |
| 4 | SystemInternal | Privileged | 51 |
| 5 | SystemInternal | Unprivileged | 43196 |

This 449 users don’t have membership in system groups.

Please, I need solution tomorrow.

				Best regards. Ruslan.

Jesse_Vincent · April 22, 2004, 7:23pm

We have 449 user who has no member ship in group Everyone!

Most of this users were created via CLI.

Can you define “created via CLI”?

Ruslan_U_Zakirov · April 23, 2004, 6:20pm

Khe-khe, folks, good time of day.
Bug located in $UserObj->Create().
This function buggy in many ways like an ‘Maasdam’ cheese.

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.
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 :)))))

If we couldn’t create a principal Id, get the fuck out.

unless ($principal_id) {
??? We check it too old.
```
  $RT::Handle->Rollback();
  $RT::Logger->crit("Couldn't create a Principal on new user 
```

create. Strange things are afoot at the circle K");
return ( 0, $self->loc(‘Could not create user’) );
}

$RT::Handle->Commit;

                                   #$RT::Logger->debug("Adding the

user as a member of everyone");
Do we realy create user? Why commit? User that doesn’t live in
’Everyone’ is not an user.

Same with Priveleged/Unpriveleged.

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

		Good luck. Ruslan.

An angry PS:

I don’t need/want/wish/like fucking users who sweem around in RT DB
like an piece of shit.
This bug once again proof that using return values for error handling
is bad practice.
I don’t know what exactly moving you guys, marketing team or somebody
else, you’re maintaining two branches at the same time. When your code
base, core of your project, has lucks in many ways which should be
refactored.
Why does RT operates so slow? It wouldn’t be fast if there will be
next code:
$RT::Logger->crit(“Deleting groups violates referential integrity
until we go through and fix this”);

TODO XXX

Remove the principal object

Remove this group from anything it’s a member of.

Remove all cached members of this group

Remove any rights granted to this group

remove any rights delegated by way of this group

return ( $self->SUPER::Delete(@_) );
Even MySQL allow FKs now, but RT still doesn’t have FKs.
Our mail relay was stopped for less then 3 days. 5000 mails were in
spool. What do you think happened when we did run it? Don’t know? Yeh,
RT didn’t live for 5 minutes, This problem not only with RT, all
together Apache/mod_perl eat all memory and begin lost mysql connection
due OOMKiller, mysql which can’t limit number threads on i386 Linux, RT
which does some things slowly. It’s fine machine with >2GHz, >1GB…

Huh, I end up. Exhausts are ended too. Nearest monthes I’ll not retry
such things

Ruslan U. Zakirov wrote:

fixup-create-user-bug (2.98 KB)

Jesse_Vincent · April 23, 2004, 8:44pm

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.

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.

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.

patch (3.73 KB)

Ruslan_U_Zakirov · April 24, 2004, 10:15am

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.

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?

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.

Jesse_Vincent · April 24, 2004, 6:45pm

Jesse Vincent пишет:

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.

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.

patch2 (2.98 KB)

MAJOR BUG in group membership

If we couldn’t create a principal Id, get the fuck out.

TODO XXX

Remove the principal object

Remove this group from anything it’s a member of.

Remove all cached members of this group

Remove any rights granted to this group

remove any rights delegated by way of this group

If we couldn’t create a principal Id, get the fuck out.

=== lib/RT/User_Overlay.pm

If we couldn’t create a principal Id, get the fuck out.

#TODO: Check if the email exists and looks valid

#TODO: Send the user a “welcome message”