[Rt-commit] rt branch, 4.2/extensible-roles, created. rt-4.0.8-606-g7e0447e
Alex Vandiver
alexmv at bestpractical.com
Thu Nov 29 18:10:12 EST 2012
The branch, 4.2/extensible-roles has been created
at 7e0447e31c1cea8e06cbff63566826a14579f24f (commit)
- Log -----------------------------------------------------------------
commit d3ba587f414adfa844da27a0ef621e63628f84c5
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Aug 28 12:15:33 2012 -0700
Convert tabs to spaces; whitespace only change
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index b367b2f..78d007c 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -187,32 +187,32 @@ Returns a user-readable description of what this group is for and what it's name
=cut
sub SelfDescription {
- my $self = shift;
- if ($self->Domain eq 'ACLEquivalence') {
- my $user = RT::Principal->new($self->CurrentUser);
- $user->Load($self->Instance);
- return $self->loc("user [_1]",$user->Object->Name);
- }
- elsif ($self->Domain eq 'UserDefined') {
- return $self->loc("group '[_1]'",$self->Name);
- }
- elsif ($self->Domain eq 'RT::System-Role') {
- return $self->loc("system [_1]",$self->Type);
- }
- elsif ($self->Domain eq 'RT::Queue-Role') {
- my $queue = RT::Queue->new($self->CurrentUser);
- $queue->Load($self->Instance);
- return $self->loc("queue [_1] [_2]",$queue->Name, $self->Type);
- }
- elsif ($self->Domain eq 'RT::Ticket-Role') {
- return $self->loc("ticket #[_1] [_2]",$self->Instance, $self->Type);
- }
- elsif ($self->Domain eq 'SystemInternal') {
- return $self->loc("system group '[_1]'",$self->Type);
- }
- else {
- return $self->loc("undescribed group [_1]",$self->Id);
- }
+ my $self = shift;
+ if ($self->Domain eq 'ACLEquivalence') {
+ my $user = RT::Principal->new($self->CurrentUser);
+ $user->Load($self->Instance);
+ return $self->loc("user [_1]",$user->Object->Name);
+ }
+ elsif ($self->Domain eq 'UserDefined') {
+ return $self->loc("group '[_1]'",$self->Name);
+ }
+ elsif ($self->Domain eq 'RT::System-Role') {
+ return $self->loc("system [_1]",$self->Type);
+ }
+ elsif ($self->Domain eq 'RT::Queue-Role') {
+ my $queue = RT::Queue->new($self->CurrentUser);
+ $queue->Load($self->Instance);
+ return $self->loc("queue [_1] [_2]",$queue->Name, $self->Type);
+ }
+ elsif ($self->Domain eq 'RT::Ticket-Role') {
+ return $self->loc("ticket #[_1] [_2]",$self->Instance, $self->Type);
+ }
+ elsif ($self->Domain eq 'SystemInternal') {
+ return $self->loc("system group '[_1]'",$self->Type);
+ }
+ else {
+ return $self->loc("undescribed group [_1]",$self->Id);
+ }
}
commit 5ae3701a5b7c1c16f2731ef2050b0b829b234b6e
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Aug 28 12:19:21 2012 -0700
LimitToRolesForTicket never worked because of non-interpolating single quotes
diff --git a/lib/RT/Groups.pm b/lib/RT/Groups.pm
index 1bb571b..26e8224 100644
--- a/lib/RT/Groups.pm
+++ b/lib/RT/Groups.pm
@@ -194,7 +194,7 @@ sub LimitToRolesForTicket {
my $self = shift;
my $Ticket = shift;
$self->Limit(FIELD => 'Domain', OPERATOR => '=', VALUE => 'RT::Ticket-Role');
- $self->Limit(FIELD => 'Instance', OPERATOR => '=', VALUE => '$Ticket');
+ $self->Limit(FIELD => 'Instance', OPERATOR => '=', VALUE => $Ticket);
}
commit 43101c2b5150fb953ade085136b9ba982d3d42f3
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Aug 28 12:24:36 2012 -0700
Handling roles on arbitrary objects in the rights editor
The rights editor now supports editing rights on arbitrary roles on any
object, not just queues and globally. (It still requires you extend RT's
core roles with your own, of course.)
Adds a new method on RT::Groups, LimitToRolesForObject, which
limits by Domain and Instance appropriately for the object given. The
new method abstracts away more of the details of roles on objects.
diff --git a/lib/RT/Groups.pm b/lib/RT/Groups.pm
index 26e8224..acfd823 100644
--- a/lib/RT/Groups.pm
+++ b/lib/RT/Groups.pm
@@ -166,11 +166,30 @@ sub LimitToUserDefinedGroups {
#$self->Limit(FIELD => 'Instance', OPERATOR => '=', VALUE => '');
}
+=head2 LimitToRolesForObject OBJECT
+Limits the set of groups to role groups specifically for the object in question
+based on the object's class and ID. If the object has no ID, the roles are not
+limited by group C<Instance>. That is, calling this method on an unloaded
+object will find all role groups for that class of object.
+Replaces L</LimitToRolesForQueue>, L</LimitToRolesForTicket>, and
+L</LimitToRolesForSystem>.
+
+=cut
+
+sub LimitToRolesForObject {
+ my $self = shift;
+ my $object = shift;
+ $self->Limit(FIELD => 'Domain', OPERATOR => '=', VALUE => ref($object) . "-Role");
+ $self->Limit(FIELD => 'Instance', OPERATOR => '=', VALUE => $object->id)
+ if $object->id and not ref($object) eq "RT::System";
+}
=head2 LimitToRolesForQueue QUEUE_ID
+B<DEPRECATED>. Use L</LimitToRolesForObject> instead.
+
Limits the set of groups found to role groups for queue QUEUE_ID
=cut
@@ -178,6 +197,7 @@ Limits the set of groups found to role groups for queue QUEUE_ID
sub LimitToRolesForQueue {
my $self = shift;
my $queue = shift;
+ RT->Logger->warning("LimitToRolesForQueue is deprecated; please change code to use LimitToRolesForObject (caller @{[join '/', caller]})");
$self->Limit(FIELD => 'Domain', OPERATOR => '=', VALUE => 'RT::Queue-Role');
$self->Limit(FIELD => 'Instance', OPERATOR => '=', VALUE => $queue);
}
@@ -186,6 +206,8 @@ sub LimitToRolesForQueue {
=head2 LimitToRolesForTicket Ticket_ID
+B<DEPRECATED>. Use L</LimitToRolesForObject> instead.
+
Limits the set of groups found to role groups for Ticket Ticket_ID
=cut
@@ -193,6 +215,7 @@ Limits the set of groups found to role groups for Ticket Ticket_ID
sub LimitToRolesForTicket {
my $self = shift;
my $Ticket = shift;
+ RT->Logger->warning("LimitToRolesForTicket is deprecated; please change code to use LimitToRolesForObject (caller @{[join '/', caller]})");
$self->Limit(FIELD => 'Domain', OPERATOR => '=', VALUE => 'RT::Ticket-Role');
$self->Limit(FIELD => 'Instance', OPERATOR => '=', VALUE => $Ticket);
}
@@ -201,12 +224,15 @@ sub LimitToRolesForTicket {
=head2 LimitToRolesForSystem System_ID
+B<DEPRECATED>. Use L</LimitToRolesForObject> instead.
+
Limits the set of groups found to role groups for System System_ID
=cut
sub LimitToRolesForSystem {
my $self = shift;
+ RT->Logger->warning("LimitToRolesForSystem is deprecated; please change code to use LimitToRolesForObject (caller @{[join '/', caller]})");
$self->Limit(FIELD => 'Domain', OPERATOR => '=', VALUE => 'RT::System-Role');
}
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index c9928c6..4df4ec6 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -3254,17 +3254,7 @@ sub GetPrincipalsMap {
}
elsif (/Roles/) {
my $roles = RT::Groups->new($session{'CurrentUser'});
-
- if ($object->isa('RT::System')) {
- $roles->LimitToRolesForSystem();
- }
- elsif ($object->isa('RT::Queue')) {
- $roles->LimitToRolesForQueue($object->Id);
- }
- else {
- $RT::Logger->warn("Skipping unknown object type ($object) for Role principals");
- next;
- }
+ $roles->LimitToRolesForObject($object);
$roles->OrderBy( FIELD => 'Type', ORDER => 'ASC' );
push @map, [
'Roles' => $roles, # loc_left_pair
commit 7e6af1c7b544c40f8c31b28566b76f0552d2aabb
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Aug 28 14:45:03 2012 -0700
Automatic friendly descriptions for arbitrary role groups
SelfDescription isn't used by core RT, but by providing generic support
in core, extensions which add role groups don't need to override the
method for any non-core code which may use it.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 78d007c..dbdc102 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -207,6 +207,11 @@ sub SelfDescription {
elsif ($self->Domain eq 'RT::Ticket-Role') {
return $self->loc("ticket #[_1] [_2]",$self->Instance, $self->Type);
}
+ elsif ($self->Domain =~ /^(.+)-Role$/) {
+ my $class = lc $1;
+ $class =~ s/^RT:://i;
+ return $self->loc("[_1] #[_2] [_3]", $self->loc($class), $self->Instance, $self->Type);
+ }
elsif ($self->Domain eq 'SystemInternal') {
return $self->loc("system group '[_1]'",$self->Type);
}
commit ffb6551681204b60aa855a923d6df8bfc1f23644
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Aug 28 15:55:49 2012 -0700
Convenience method to load arbitrary role groups based on an object
Rather than requiring the caller to discern the class of object and call
one of the class specific load methods, just pass in the object itself
and be happy.
RT::Test->add_rights now uses this and as such supports granting rights
to role groups on objects other than queues or the global system.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index dbdc102..d6bcd93 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -316,6 +316,29 @@ sub LoadSystemInternalGroup {
);
}
+=head2 LoadRoleGroup
+
+Takes a paramhash of Object and Type and attempts to load the suitable role
+group for said object.
+
+=cut
+
+sub LoadRoleGroup {
+ my $self = shift;
+ my %args = (
+ Object => undef,
+ Type => undef,
+ @_
+ );
+
+ # Translate Object to Domain + Instance
+ my $object = delete $args{Object};
+ $args{Domain} = ref($object) . "-Role";
+ $args{Instance} = $object->id
+ if $object->id and not ref($object) eq 'RT::System';
+
+ return $self->LoadByCols(%args);
+}
=head2 LoadTicketRoleGroup { Ticket => TICKET_ID, Type => TYPE }
diff --git a/lib/RT/Test.pm b/lib/RT/Test.pm
index aafeebc..5daeea0 100644
--- a/lib/RT/Test.pm
+++ b/lib/RT/Test.pm
@@ -922,16 +922,16 @@ sub add_rights {
if ( $principal =~ /^(everyone|(?:un)?privileged)$/i ) {
$principal = RT::Group->new( RT->SystemUser );
$principal->LoadSystemInternalGroup($1);
- } elsif ( $principal =~ /^(Owner|Requestor|(?:Admin)?Cc)$/i ) {
+ } else {
+ my $type = $principal;
$principal = RT::Group->new( RT->SystemUser );
- $principal->LoadByCols(
- Domain => (ref($e->{'Object'})||'RT::System').'-Role',
- Type => $1,
- ref($e->{'Object'})? (Instance => $e->{'Object'}->id): (),
+ $principal->LoadRoleGroup(
+ Object => ($e->{'Object'} || RT->System),
+ Type => $type
);
- } else {
- die "principal is not an object, but also is not name of a system group";
}
+ die "Principal is not an object nor the name of a system or role group"
+ unless $principal->id;
}
unless ( $principal->isa('RT::Principal') ) {
if ( $principal->can('PrincipalObj') ) {
commit 9b8b2fa00b393ff784beb8276f6373749df9671e
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Sep 13 17:13:16 2012 -0700
CreateRoleGroup can take an Object in place of Domain and Instance
Paves the way for easier creation of role groups on arbitrary objects.
Flesh out the documentation for CreateRoleGroup while I'm at it.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index d6bcd93..f0049cb 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -606,16 +606,47 @@ sub _CreateACLEquivalenceGroup {
-=head2 CreateRoleGroup { Domain => DOMAIN, Type => TYPE, Instance => ID }
+=head2 CreateRoleGroup
-A helper subroutine which creates a ticket group. (What RT 2.0 called Ticket watchers)
-Type is one of ( "Requestor" || "Cc" || "AdminCc" || "Owner")
-Domain is one of (RT::Ticket-Role || RT::Queue-Role || RT::System-Role)
-Instance is the id of the ticket or queue in question
+A convenience method for creating a role group on an object.
-This routine expects to be called from {Ticket||Queue}->CreateTicketGroups _inside of a transaction_
+Takes a paramhash of:
-Returns a tuple of (Id, Message). If id is 0, the create failed
+=over 4
+
+=item Type
+
+Required. RT's core role types are C<Requestor>, C<Cc>, C<AdminCc>, and
+C<Owner>. Extensions may add their own.
+
+=item Object
+
+Optional. The object on which this role applies, used to set Domain and
+Instance automatically.
+
+=item Domain
+
+Optional. The class on which this role applies, with C<-Role> appended. RT's
+supported core role group domains are C<RT::Ticket-Role>, C<RT::Queue-Role>,
+and C<RT::System-Role>.
+
+Not required if you pass an Object.
+
+=item Instance
+
+Optional. The numeric ID of the object (of the class encoded in Domain) on
+which this role applies. If Domain is C<RT::System-Role>, Instance should be C<0>.
+
+Not required if you pass an Object.
+
+=back
+
+You must pass either an Object or both Domain and Instance.
+
+This method must be called from B<inside of a database transaction>!
+
+Returns a tuple of (id, Message). If id is false, the create failed and
+Message should contain an error string.
=cut
@@ -624,17 +655,24 @@ sub CreateRoleGroup {
my %args = ( Instance => undef,
Type => undef,
Domain => undef,
+ Object => undef,
@_ );
+ # Translate Object to Domain + Instance
+ if ( my $object = delete $args{Object} ) {
+ $args{Domain} = ref($object) . "-Role";
+ $args{Instance} = ref($object) eq "RT::System" ? 0 : $object->id;
+ }
+
unless (RT::Queue->IsRoleGroupType($args{Type})) {
return ( 0, $self->loc("Invalid Group Type") );
}
+ return $self->_Create(
+ InsideTransaction => 1,
+ map { $_ => $args{$_} } qw(Domain Instance Type),
+ );
- return ( $self->_Create( Domain => $args{'Domain'},
- Instance => $args{'Instance'},
- Type => $args{'Type'},
- InsideTransaction => 1 ) );
}
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index f6ec266..6cd053f 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -814,9 +814,10 @@ sub _CreateQueueRoleGroup {
my $type = shift;
my $type_obj = RT::Group->new($self->CurrentUser);
- my ($id, $msg) = $type_obj->CreateRoleGroup(Instance => $self->Id,
- Type => $type,
- Domain => 'RT::Queue-Role');
+ my ($id, $msg) = $type_obj->CreateRoleGroup(
+ Type => $type,
+ Object => $self,
+ );
unless ($id) {
$RT::Logger->error("Couldn't create a Queue group of type '$type' for queue ".
$self->Id.": ".$msg);
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 745789e..ead305b 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -948,9 +948,10 @@ sub _CreateTicketGroups {
foreach my $type (@types) {
my $type_obj = RT::Group->new($self->CurrentUser);
- my ($id, $msg) = $type_obj->CreateRoleGroup(Domain => 'RT::Ticket-Role',
- Instance => $self->Id,
- Type => $type);
+ my ($id, $msg) = $type_obj->CreateRoleGroup(
+ Type => $type,
+ Object => $self,
+ );
unless ($id) {
$RT::Logger->error("Couldn't create a ticket group of type '$type' for ticket ".
$self->Id.": ".$msg);
commit d58e81c285c210df881cdfd43f18e42d4be47a40
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Sep 13 17:16:10 2012 -0700
Don't assume new role groups are for tickets/queues
CreateRoleGroup no longer assumes the role is one RT::Queue will know
about when validating. Instead, RT::Group maintains a map of role names
to valid classes. The core roles are hardcoded, but extensions are free
to push into the %ROLES package variable and start using CreateRoleGroup
for roles on other object types.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index f0049cb..f5c93d6 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -62,12 +62,6 @@ my $group = RT::Group->new($CurrentUser);
An RT group object.
-=head1 METHODS
-
-
-
-
-
=cut
@@ -119,8 +113,35 @@ $RIGHT_CATEGORIES = {
# Tell RT::ACE that this sort of object can get acls granted
$RT::ACE::OBJECT_TYPES{'RT::Group'} = 1;
+=head1 PACKAGE VARIABLES
+
+=head2 %ROLES
+
+If you need to know the roles provided by a class, use L</RolesOf>. If you
+want to validate a role type and domain pair, use L</ValidateRoleGroup>.
+
+B<You should not need to touch this variable directly unless you're adding new
+roles into RT.>
+
+Defines which roles are valid for which classes. In terms of groups, this maps
+group Types to Domains (minus the C<-Role> suffix), providing valid (Domain,
+Type) pairs.
+
+L<RT::System> is implicitly valid for all roles, as the ACL system assumes such
+and there's not much value requiring it repeated here.
+
+The right hand side essentially lists the class names for the possible
+ACLEquivalenceObjects.
+
+=cut
+
+our %ROLES = (
+ Requestor => [qw(RT::Ticket RT::Queue)],
+ Cc => [qw(RT::Ticket RT::Queue)],
+ Owner => [qw(RT::Ticket RT::Queue)],
+ AdminCc => [qw(RT::Ticket RT::Queue)],
+);
-#
# TODO: This should be refactored out into an RT::ACLedObject or something
# stuff the rights into a hash of rights that can exist.
@@ -128,6 +149,8 @@ $RT::ACE::OBJECT_TYPES{'RT::Group'} = 1;
__PACKAGE__->AddRights(%$RIGHTS);
__PACKAGE__->AddRightCategories(%$RIGHT_CATEGORIES);
+=head1 METHODS
+
=head2 AddRights C<RIGHT>, C<DESCRIPTION> [, ...]
Adds the given rights to the list of possible rights. This method
@@ -664,18 +687,63 @@ sub CreateRoleGroup {
$args{Instance} = ref($object) eq "RT::System" ? 0 : $object->id;
}
- unless (RT::Queue->IsRoleGroupType($args{Type})) {
- return ( 0, $self->loc("Invalid Group Type") );
+ # XXX I WISH: If this was Moose and we had Roles to implement roles, we'd
+ # take a class or object, check $class->DOES('ACLRole'), and then call
+ # $class->IsValidRole($Type) or similar if DOES was true.
+ unless ($self->ValidateRoleGroup(%args)) {
+ return ( 0, $self->loc("Invalid Group Type and Domain") );
}
return $self->_Create(
InsideTransaction => 1,
map { $_ => $args{$_} } qw(Domain Instance Type),
);
+}
+
+=head2 ValidateRoleGroup
+
+Takes a param hash containing Domain and Type which are expected to be values
+passed into L</CreateRoleGroup>. Returns true if the specified Type is a valid
+role on the specified Domain. Otherwise returns false.
+
+All roles are valid for the global Domain (C<RT::System-Role>).
+
+=cut
+
+sub ValidateRoleGroup {
+ my $self = shift;
+ my %args = (@_);
+ return 0 unless $args{Domain} and $args{Type};
+
+ my $classes = $ROLES{ $args{Type} };
+ return 0 unless $classes and ref($classes) eq 'ARRAY';
+ my ($domain) = $args{Domain} =~ /^(.+)-Role$/;
+
+ # All roles are valid for the global domain (RT::System), and we've already
+ # determined this is a role defined in %ROLES.
+ return 1 if $domain eq "RT::System";
+ return 1 if grep { $_ eq $domain } @$classes;
+ return 0;
}
+=head2 RolesOf
+
+Takes a class name or object, and returns the names of the roles which apply to
+the class.
+=cut
+
+sub RolesOf {
+ my $self = shift;
+ my $class = ref($_[0]) || $_[0];
+ my @roles;
+ for my $role (keys %ROLES) {
+ push @roles, $role
+ if grep { $_ eq $class } @{$ROLES{$role}};
+ }
+ return @roles;
+}
=head2 Delete
commit 270353574bccbf9814dac3963b9c133a27138f1f
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Sep 13 17:23:09 2012 -0700
Implement RT::Queue's methods about roles using the new methods of RT::Group
Now extensions can add new roles to queues/tickets as well, at least at
the API level.
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 6cd053f..90b319d 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -727,15 +727,22 @@ sub TicketTransactionCustomFields {
=head2 AllRoleGroupTypes
-Returns a list of the names of the various role group types that this queue
-has, including Requestor and Owner. If you don't want them, see
-L</ManageableRoleGroupTypes>.
+B<DEPRECATED> and will be removed in a future release. Use L<RT::Group/RolesOf>
+instead.
+
+Returns a list of the names of the various role group types for Queues,
+including roles used only for ACLs like Requestor and Owner. If you don't want
+them, see L</ManageableRoleGroupTypes>.
=cut
sub AllRoleGroupTypes {
- my $self = shift;
- return ($self->ManageableRoleGroupTypes, qw(Requestor Owner));
+ RT->Logger->warn(<<" .");
+RT::Queue->AllRoleGroupTypes is DEPRECATED and will be removed in a future release.
+
+Please use RT::Group->RolesOf('RT::Queue') instead at @{[join '/', caller]}.
+ .
+ RT::Group->RolesOf('RT::Queue')
}
=head2 IsRoleGroupType
@@ -748,22 +755,24 @@ sub IsRoleGroupType {
my $self = shift;
my $type = shift;
- for my $valid_type ($self->AllRoleGroupTypes) {
- return 1 if $type eq $valid_type;
- }
-
- return 0;
+ return RT::Group->ValidateRoleGroup(
+ Domain => 'RT::Queue-Role',
+ Type => $type,
+ );
}
=head2 ManageableRoleGroupTypes
-Returns a list of the names of the various role group types that this queue
-has, excluding Requestor and Owner. If you want them, see L</AllRoleGroupTypes>.
+Returns a list of the names of the various role group types for Queues,
+excluding ones used only for ACLs such as Requestor and Owner. If you want
+them, see L<RT::Group/RolesOf>.
=cut
sub ManageableRoleGroupTypes {
- return qw(Cc AdminCc);
+ # This grep is a little hacky, but I don't want to introduce the concept of
+ # manageable vs. unmanageable roles globally (yet).
+ return grep { not /^(Requestor|Owner)$/ } RT::Group->RolesOf('RT::Queue');
}
=head2 IsManageableRoleGroupType
@@ -799,9 +808,7 @@ It will return true on success and undef on failure.
sub _CreateQueueGroups {
my $self = shift;
- my @types = $self->AllRoleGroupTypes;
-
- foreach my $type (@types) {
+ foreach my $type (RT::Group->RolesOf($self)) {
my $ok = $self->_CreateQueueRoleGroup($type);
return undef if !$ok;
}
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index ead305b..06f651a 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -944,9 +944,7 @@ It will return true on success and undef on failure.
sub _CreateTicketGroups {
my $self = shift;
- my @types = (qw(Requestor Owner Cc AdminCc));
-
- foreach my $type (@types) {
+ foreach my $type (RT::Group->RolesOf($self)) {
my $type_obj = RT::Group->new($self->CurrentUser);
my ($id, $msg) = $type_obj->CreateRoleGroup(
Type => $type,
commit 123bf570efc47bd71c1b2f1479cb2139dc928068
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Sep 18 12:40:04 2012 -0700
Refactor role registration metadata into RT::Record
The RegisterRole method replaces pushing into %RT::Group::ROLES and
knows how to handle equivalent classes and RT::System. This allows
greater flexibility to add metadata in the future (such as manageable
vs. unmanageable role types).
RT::Record->Roles replaces RT::Group->RolesOf(RT::Record).
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index f5c93d6..1f48df1 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -113,36 +113,6 @@ $RIGHT_CATEGORIES = {
# Tell RT::ACE that this sort of object can get acls granted
$RT::ACE::OBJECT_TYPES{'RT::Group'} = 1;
-=head1 PACKAGE VARIABLES
-
-=head2 %ROLES
-
-If you need to know the roles provided by a class, use L</RolesOf>. If you
-want to validate a role type and domain pair, use L</ValidateRoleGroup>.
-
-B<You should not need to touch this variable directly unless you're adding new
-roles into RT.>
-
-Defines which roles are valid for which classes. In terms of groups, this maps
-group Types to Domains (minus the C<-Role> suffix), providing valid (Domain,
-Type) pairs.
-
-L<RT::System> is implicitly valid for all roles, as the ACL system assumes such
-and there's not much value requiring it repeated here.
-
-The right hand side essentially lists the class names for the possible
-ACLEquivalenceObjects.
-
-=cut
-
-our %ROLES = (
- Requestor => [qw(RT::Ticket RT::Queue)],
- Cc => [qw(RT::Ticket RT::Queue)],
- Owner => [qw(RT::Ticket RT::Queue)],
- AdminCc => [qw(RT::Ticket RT::Queue)],
-);
-
-
# TODO: This should be refactored out into an RT::ACLedObject or something
# stuff the rights into a hash of rights that can exist.
@@ -703,10 +673,8 @@ sub CreateRoleGroup {
=head2 ValidateRoleGroup
Takes a param hash containing Domain and Type which are expected to be values
-passed into L</CreateRoleGroup>. Returns true if the specified Type is a valid
-role on the specified Domain. Otherwise returns false.
-
-All roles are valid for the global Domain (C<RT::System-Role>).
+passed into L</CreateRoleGroup>. Returns true if the specified Type is a
+registered role on the specified Domain. Otherwise returns false.
=cut
@@ -715,36 +683,13 @@ sub ValidateRoleGroup {
my %args = (@_);
return 0 unless $args{Domain} and $args{Type};
- my $classes = $ROLES{ $args{Type} };
- return 0 unless $classes and ref($classes) eq 'ARRAY';
-
- my ($domain) = $args{Domain} =~ /^(.+)-Role$/;
+ my ($class) = $args{Domain} =~ /^(.+)-Role$/;
+ return 0 unless $class and $class->can('Roles');
- # All roles are valid for the global domain (RT::System), and we've already
- # determined this is a role defined in %ROLES.
- return 1 if $domain eq "RT::System";
- return 1 if grep { $_ eq $domain } @$classes;
+ return 1 if grep { $args{Type} eq $_ } $class->Roles;
return 0;
}
-=head2 RolesOf
-
-Takes a class name or object, and returns the names of the roles which apply to
-the class.
-
-=cut
-
-sub RolesOf {
- my $self = shift;
- my $class = ref($_[0]) || $_[0];
- my @roles;
- for my $role (keys %ROLES) {
- push @roles, $role
- if grep { $_ eq $class } @{$ROLES{$role}};
- }
- return @roles;
-}
-
=head2 Delete
Delete this object
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 90b319d..24d7e60 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -727,7 +727,7 @@ sub TicketTransactionCustomFields {
=head2 AllRoleGroupTypes
-B<DEPRECATED> and will be removed in a future release. Use L<RT::Group/RolesOf>
+B<DEPRECATED> and will be removed in a future release. Use L</Roles>
instead.
Returns a list of the names of the various role group types for Queues,
@@ -740,9 +740,9 @@ sub AllRoleGroupTypes {
RT->Logger->warn(<<" .");
RT::Queue->AllRoleGroupTypes is DEPRECATED and will be removed in a future release.
-Please use RT::Group->RolesOf('RT::Queue') instead at @{[join '/', caller]}.
+Please use RT::Queue->Roles instead at @{[join '/', caller]}.
.
- RT::Group->RolesOf('RT::Queue')
+ shift->Roles;
}
=head2 IsRoleGroupType
@@ -765,14 +765,14 @@ sub IsRoleGroupType {
Returns a list of the names of the various role group types for Queues,
excluding ones used only for ACLs such as Requestor and Owner. If you want
-them, see L<RT::Group/RolesOf>.
+them, see L</Roles>.
=cut
sub ManageableRoleGroupTypes {
# This grep is a little hacky, but I don't want to introduce the concept of
# manageable vs. unmanageable roles globally (yet).
- return grep { not /^(Requestor|Owner)$/ } RT::Group->RolesOf('RT::Queue');
+ return grep { not /^(Requestor|Owner)$/ } shift->Roles;
}
=head2 IsManageableRoleGroupType
@@ -808,7 +808,7 @@ It will return true on success and undef on failure.
sub _CreateQueueGroups {
my $self = shift;
- foreach my $type (RT::Group->RolesOf($self)) {
+ foreach my $type ($self->Roles) {
my $ok = $self->_CreateQueueRoleGroup($type);
return undef if !$ok;
}
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 21905a3..91e7185 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2040,6 +2040,77 @@ sub WikiBase {
return RT->Config->Get('WebPath'). "/index.html?q=";
}
+=head2 RegisterRole
+
+Registers an RT role which applies to this class for role-based access control.
+Arguments:
+
+=over 4
+
+=item Name
+
+Required. The role name (i.e. Requestor, Owner, AdminCc, etc).
+
+=item EquivClasses
+
+Optional. Array ref of classes through which this role percolates up to
+L<RT::System>. You can think of this list as:
+
+ map { ref } $record_object->ACLEquivalenceObjects;
+
+You should not include L<RT::System> itself in this list.
+
+Simply calls RegisterRole on each equivalent class.
+
+=back
+
+=cut
+
+sub RegisterRole {
+ my $self = shift;
+ my $class = ref($self) || $self;
+ my %role = (
+ Name => undef,
+ EquivClasses => [],
+ @_
+ );
+ return unless $role{Name};
+
+ # Stash the role on ourself
+ $class->_ROLES->{ $role{Name} } = \%role;
+
+ # Register it with any equivalent classes...
+ my $equiv = delete $role{EquivClasses} || [];
+
+ # ... and globally unless we ARE global
+ unless ($class eq "RT::System") {
+ require RT::System;
+ push @$equiv, "RT::System";
+ }
+
+ $_->RegisterRole(%role) for @$equiv;
+
+ # XXX TODO: Register which classes have roles on them somewhere?
+
+ return 1;
+}
+
+=head2 Roles
+
+Returns a list of role names registered for this class.
+
+=cut
+
+sub Roles { sort { $a cmp $b } keys %{ shift->_ROLES } }
+
+{
+ my %ROLES;
+ sub _ROLES {
+ my $class = ref($_[0]) || $_[0];
+ return $ROLES{$class} ||= {};
+ }
+}
+
RT::Base->_ImportOverlays();
1;
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 06f651a..e89d4b8 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -84,6 +84,13 @@ use RT::URI;
use MIME::Entity;
use Devel::GlobalDestruction;
+for my $role (qw(Requestor Cc AdminCc Owner)) {
+ RT::Ticket->RegisterRole(
+ Name => $role,
+ EquivClasses => ['RT::Queue'],
+ );
+}
+
our %MERGE_CACHE = (
effective => {},
merged => {},
@@ -944,7 +951,7 @@ It will return true on success and undef on failure.
sub _CreateTicketGroups {
my $self = shift;
- foreach my $type (RT::Group->RolesOf($self)) {
+ foreach my $type ($self->Roles) {
my $type_obj = RT::Group->new($self->CurrentUser);
my ($id, $msg) = $type_obj->CreateRoleGroup(
Type => $type,
commit fbb087d0817a843b9572d5b53188e511eed4ed6b
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Sep 18 13:14:40 2012 -0700
HasRole method to check role existence on a class
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 1f48df1..f8414b7 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -657,9 +657,6 @@ sub CreateRoleGroup {
$args{Instance} = ref($object) eq "RT::System" ? 0 : $object->id;
}
- # XXX I WISH: If this was Moose and we had Roles to implement roles, we'd
- # take a class or object, check $class->DOES('ACLRole'), and then call
- # $class->IsValidRole($Type) or similar if DOES was true.
unless ($self->ValidateRoleGroup(%args)) {
return ( 0, $self->loc("Invalid Group Type and Domain") );
}
@@ -684,10 +681,9 @@ sub ValidateRoleGroup {
return 0 unless $args{Domain} and $args{Type};
my ($class) = $args{Domain} =~ /^(.+)-Role$/;
- return 0 unless $class and $class->can('Roles');
+ return 0 unless $class and $class->can('HasRole');
- return 1 if grep { $args{Type} eq $_ } $class->Roles;
- return 0;
+ return $class->HasRole($args{Type});
}
=head2 Delete
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 24d7e60..4bcaed5 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -747,18 +747,19 @@ Please use RT::Queue->Roles instead at @{[join '/', caller]}.
=head2 IsRoleGroupType
+B<DEPRECATED> and will be removed in a future release. Use L</HasRole> instead.
+
Returns whether the passed-in type is a role group type.
=cut
sub IsRoleGroupType {
- my $self = shift;
- my $type = shift;
+ RT->Logger->warn(<<" .");
+RT::Queue->IsRoleGroupType is DEPRECATED and will be removed in a future release.
- return RT::Group->ValidateRoleGroup(
- Domain => 'RT::Queue-Role',
- Type => $type,
- );
+Please use RT::Queue->HasRole instead at @{[join '/', caller]}.
+ .
+ shift->HasRole(@_);
}
=head2 ManageableRoleGroupTypes
@@ -901,7 +902,7 @@ sub AddWatcher {
}
return ( 0, "Unknown watcher type [_1]", $args{Type} )
- unless $self->IsRoleGroupType($args{Type});
+ unless $self->HasRole($args{Type});
my ($ok, $msg) = $self->_HasModifyWatcherRight(%args);
return ($ok, $msg) if !$ok;
@@ -1050,7 +1051,7 @@ sub DeleteWatcher {
}
return ( 0, $self->loc('Unknown watcher type [_1]', $args{Type}) )
- unless $self->IsRoleGroupType($args{Type});
+ unless $self->HasRole($args{Type});
my ($ok, $msg) = $self->_HasModifyWatcherRight(%args);
return ($ok, $msg) if !$ok;
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 91e7185..3a22d77 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2111,6 +2111,19 @@ sub Roles { sort { $a cmp $b } keys %{ shift->_ROLES } }
}
}
+=head2 HasRole
+
+Returns true if the name provided is a registered role for this class.
+Otherwise returns false.
+
+=cut
+
+sub HasRole {
+ my $self = shift;
+ my $type = shift;
+ return scalar grep { $type eq $_ } $self->Roles;
+}
+
RT::Base->_ImportOverlays();
1;
commit f54a4abb564937873d825b423f00fa9a47ffd608
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Sep 18 14:24:28 2012 -0700
Bail out of LoadRoleGroup if the object passed is unloaded
Avoids loading up a random role instance.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index f8414b7..563a84e 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -324,11 +324,16 @@ sub LoadRoleGroup {
@_
);
+ my $object = delete $args{Object};
+
+ return (0, $self->loc("Object passed is not loaded"))
+ if ref($object) ne "RT::System"
+ and not $object->id;
+
# Translate Object to Domain + Instance
- my $object = delete $args{Object};
$args{Domain} = ref($object) . "-Role";
$args{Instance} = $object->id
- if $object->id and not ref($object) eq 'RT::System';
+ unless ref($object) eq 'RT::System';
return $self->LoadByCols(%args);
}
commit 13f7c1e22f14b12c51765c0568c4dffa96fe050f
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Sep 18 14:28:51 2012 -0700
Require Instance (even if it's 0) when creating a new role group
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 563a84e..3260e34 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -662,6 +662,10 @@ sub CreateRoleGroup {
$args{Instance} = ref($object) eq "RT::System" ? 0 : $object->id;
}
+ unless (defined $args{Instance}) {
+ return ( 0, $self->loc("An Instance must be provided") );
+ }
+
unless ($self->ValidateRoleGroup(%args)) {
return ( 0, $self->loc("Invalid Group Type and Domain") );
}
commit 7663a52c6b131ff0d4bf779782f94b8e94022ac5
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Sep 18 14:37:50 2012 -0700
Block duplicate role group creation
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 3260e34..df25d68 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -670,9 +670,17 @@ sub CreateRoleGroup {
return ( 0, $self->loc("Invalid Group Type and Domain") );
}
+ my %create = map { $_ => $args{$_} } qw(Domain Instance Type);
+
+ my $duplicate = RT::Group->new( RT->SystemUser );
+ $duplicate->LoadByCols( %create );
+ if ($duplicate->id) {
+ return ( 0, $self->loc("Role group exists already") );
+ }
+
return $self->_Create(
InsideTransaction => 1,
- map { $_ => $args{$_} } qw(Domain Instance Type),
+ %create,
);
}
commit c5e48f46f3f8059c2a2de9d7832147516adb3d89
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Sep 18 15:49:45 2012 -0700
Set system role groups' Instance to RT->System->id
These role groups are now consistent with other role groups instead of
using the legacy Instance value of 0 (which doesn't match
RT->System->id).
diff --git a/etc/upgrade/4.1.4/schema.Oracle b/etc/upgrade/4.1.4/schema.Oracle
new file mode 100644
index 0000000..e530ede
--- /dev/null
+++ b/etc/upgrade/4.1.4/schema.Oracle
@@ -0,0 +1 @@
+UPDATE Groups SET Instance = 1 WHERE Domain = 'RT::System-Role' AND Instance = 0;
diff --git a/etc/upgrade/4.1.4/schema.Pg b/etc/upgrade/4.1.4/schema.Pg
new file mode 100644
index 0000000..e530ede
--- /dev/null
+++ b/etc/upgrade/4.1.4/schema.Pg
@@ -0,0 +1 @@
+UPDATE Groups SET Instance = 1 WHERE Domain = 'RT::System-Role' AND Instance = 0;
diff --git a/etc/upgrade/4.1.4/schema.SQLite b/etc/upgrade/4.1.4/schema.SQLite
new file mode 100644
index 0000000..e530ede
--- /dev/null
+++ b/etc/upgrade/4.1.4/schema.SQLite
@@ -0,0 +1 @@
+UPDATE Groups SET Instance = 1 WHERE Domain = 'RT::System-Role' AND Instance = 0;
diff --git a/etc/upgrade/4.1.4/schema.mysql b/etc/upgrade/4.1.4/schema.mysql
new file mode 100644
index 0000000..e530ede
--- /dev/null
+++ b/etc/upgrade/4.1.4/schema.mysql
@@ -0,0 +1 @@
+UPDATE Groups SET Instance = 1 WHERE Domain = 'RT::System-Role' AND Instance = 0;
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index df25d68..7700a95 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -327,13 +327,11 @@ sub LoadRoleGroup {
my $object = delete $args{Object};
return (0, $self->loc("Object passed is not loaded"))
- if ref($object) ne "RT::System"
- and not $object->id;
+ unless $object->id;
# Translate Object to Domain + Instance
$args{Domain} = ref($object) . "-Role";
- $args{Instance} = $object->id
- unless ref($object) eq 'RT::System';
+ $args{Instance} = $object->id;
return $self->LoadByCols(%args);
}
@@ -633,7 +631,7 @@ Not required if you pass an Object.
=item Instance
Optional. The numeric ID of the object (of the class encoded in Domain) on
-which this role applies. If Domain is C<RT::System-Role>, Instance should be C<0>.
+which this role applies. If Domain is C<RT::System-Role>, Instance should be C<1>.
Not required if you pass an Object.
@@ -659,10 +657,10 @@ sub CreateRoleGroup {
# Translate Object to Domain + Instance
if ( my $object = delete $args{Object} ) {
$args{Domain} = ref($object) . "-Role";
- $args{Instance} = ref($object) eq "RT::System" ? 0 : $object->id;
+ $args{Instance} = $object->id;
}
- unless (defined $args{Instance}) {
+ unless ($args{Instance}) {
return ( 0, $self->loc("An Instance must be provided") );
}
diff --git a/lib/RT/Handle.pm b/lib/RT/Handle.pm
index 58934da..88e8fb3 100644
--- a/lib/RT/Handle.pm
+++ b/lib/RT/Handle.pm
@@ -715,12 +715,10 @@ sub InsertInitialData {
}
$group = RT::Group->new( RT->SystemUser );
- my ( $val, $msg ) = $group->_Create(
+ my ( $val, $msg ) = $group->CreateRoleGroup(
Type => $name,
- Domain => 'RT::System-Role',
+ Object => RT->System,
Description => 'SystemRolegroup for internal use', # loc
- Name => '',
- Instance => '',
);
return ($val, $msg) unless $val;
}
commit 72524ea21892c6d6dd39816581bd30448573d42a
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Sep 19 14:08:55 2012 -0700
Group.Instance is numeric, drop quoting and incorrect comment
This comment used to be true but became false many moons ago.
diff --git a/lib/RT/Principal.pm b/lib/RT/Principal.pm
index 4c225c4..9520ef0 100644
--- a/lib/RT/Principal.pm
+++ b/lib/RT/Principal.pm
@@ -577,11 +577,8 @@ sub _HasRoleRightQuery {
my $clause = "Groups.Domain = '$type-Role'";
- # XXX: Groups.Instance is VARCHAR in DB, we should quote value
- # if we want mysql 4.0 use indexes here. we MUST convert that
- # field to integer and drop this quotes.
if ( my $id = eval { $obj->id } ) {
- $clause .= " AND Groups.Instance = '$id'";
+ $clause .= " AND Groups.Instance = $id";
}
push @object_clauses, "($clause)";
}
diff --git a/lib/RT/Users.pm b/lib/RT/Users.pm
index 787ac10..d56f4ec 100644
--- a/lib/RT/Users.pm
+++ b/lib/RT/Users.pm
@@ -461,10 +461,7 @@ sub _RoleClauses {
$id = $obj->id if ref($obj) && UNIVERSAL::can($obj, 'id') && $obj->id;
my $role_clause = "$groups.Domain = '$type-Role'";
- # XXX: Groups.Instance is VARCHAR in DB, we should quote value
- # if we want mysql 4.0 use indexes here. we MUST convert that
- # field to integer and drop this quotes.
- $role_clause .= " AND $groups.Instance = '$id'" if $id;
+ $role_clause .= " AND $groups.Instance = $id" if $id;
push @groups_clauses, "($role_clause)";
}
return @groups_clauses;
commit fb5df38c7e15393bac386aa5e2b9fc2f77b65fc0
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Sep 19 16:09:26 2012 -0700
Document transaction requirement of CreateRoleGroup and follow our own advice
System role groups weren't created inside a transaction despite role
group creation expecting to be able to Rollback. Document how to toggle
the expected transaction state and then use it in
RT::Handle->InsertInitialData.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 7700a95..f5548ed 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -606,6 +606,10 @@ sub _CreateACLEquivalenceGroup {
A convenience method for creating a role group on an object.
+This method expects to be called from B<inside of a database transaction>! If
+you're calling it outside of one, you B<MUST> pass a false value for
+InsideTransaction.
+
Takes a paramhash of:
=over 4
@@ -635,12 +639,16 @@ which this role applies. If Domain is C<RT::System-Role>, Instance should be C<
Not required if you pass an Object.
+=item InsideTransaction
+
+Optional. Defaults to true in expectation of usual call sites. If you call
+this method while not inside a transaction, you C<MUST> pass a false value for
+this parameter.
+
=back
You must pass either an Object or both Domain and Instance.
-This method must be called from B<inside of a database transaction>!
-
Returns a tuple of (id, Message). If id is false, the create failed and
Message should contain an error string.
diff --git a/lib/RT/Handle.pm b/lib/RT/Handle.pm
index 88e8fb3..2b57d0b 100644
--- a/lib/RT/Handle.pm
+++ b/lib/RT/Handle.pm
@@ -716,9 +716,10 @@ sub InsertInitialData {
$group = RT::Group->new( RT->SystemUser );
my ( $val, $msg ) = $group->CreateRoleGroup(
- Type => $name,
- Object => RT->System,
- Description => 'SystemRolegroup for internal use', # loc
+ Type => $name,
+ Object => RT->System,
+ Description => 'SystemRolegroup for internal use', # loc
+ InsideTransaction => 0,
);
return ($val, $msg) unless $val;
}
commit 77dbbcbdc1cda601c7443cbd0324983520b4d424
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Sep 19 20:07:07 2012 -0700
Refactor role group check on Principal into a public method
Greater abstraction of the /-Role$/ check means easier changes down the
road as necessary.
diff --git a/lib/RT/Principal.pm b/lib/RT/Principal.pm
index 9520ef0..4c5a4c4 100644
--- a/lib/RT/Principal.pm
+++ b/lib/RT/Principal.pm
@@ -88,7 +88,18 @@ sub IsGroup {
return undef;
}
+=head2 IsRoleGroup
+Returns true if this principal is a role group.
+Returns undef, otherwise.
+
+=cut
+
+sub IsRoleGroup {
+ my $self = shift;
+ return ($self->IsGroup and $self->Object->Domain =~ /-Role$/)
+ ? 1 : undef;
+}
=head2 IsUser
@@ -698,7 +709,7 @@ return that. if it has no type, return group.
sub _GetPrincipalTypeForACL {
my $self = shift;
- if ($self->PrincipalType eq 'Group' && $self->Object->Domain =~ /Role$/) {
+ if ($self->IsRoleGroup) {
return $self->Object->Type;
} else {
return $self->PrincipalType;
commit d8b7270f7a5a7aecc04335d0a668a62fa9ac82cc
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Sep 19 20:14:25 2012 -0700
Sort object types for stability when combining ACE hashes
Some rights are announced by multiple classes but currently have
different descriptions. Sorting at least makes the descriptions stable
in the global rights list.
diff --git a/lib/RT/System.pm b/lib/RT/System.pm
index 68f1564..558e98e 100644
--- a/lib/RT/System.pm
+++ b/lib/RT/System.pm
@@ -138,7 +138,7 @@ sub _ForAllACEObjectTypes {
return {} unless $method;
my %data;
- for my $class (keys %RT::ACE::OBJECT_TYPES) {
+ for my $class (sort keys %RT::ACE::OBJECT_TYPES) {
next unless $RT::ACE::OBJECT_TYPES{$class};
# Skip ourselves otherwise we'd loop infinitely
commit 07e7ce98c0132ea810c0abb81c36105fa001cde7
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Sep 19 20:31:54 2012 -0700
Restrict rights granted to system role groups
All roles include a system-level group which is used for global role
rights. System level rights are a combination of global-only rights
(those announced in RT::System itself) and rights announced by other
ACL-able classes (%RT::ACE::OBJECT_TYPES). System role groups, however,
are only registered to certain classes, and displaying or allowing
granting of rights announced by classes other than those to which the
role is registered makes no sense as those rights will never be checked.
This only matters for system role groups because AvailableRights in
other classes is restricted to the correct class by virtue of being in
the correct class itself.
diff --git a/lib/RT/ACE.pm b/lib/RT/ACE.pm
index c878a2d..914670d 100644
--- a/lib/RT/ACE.pm
+++ b/lib/RT/ACE.pm
@@ -266,7 +266,7 @@ sub Create {
#check if it's a valid RightName
if ( $args{'Object'}->can('AvailableRights') ) {
- my $available = $args{'Object'}->AvailableRights;
+ my $available = $args{'Object'}->AvailableRights($princ_obj);
unless ( grep $_ eq $args{'RightName'}, map $self->CanonicalizeRightName( $_ ), keys %$available ) {
$RT::Logger->warning(
"Couldn't validate right name '$args{'RightName'}'"
diff --git a/lib/RT/System.pm b/lib/RT/System.pm
index 558e98e..28fea07 100644
--- a/lib/RT/System.pm
+++ b/lib/RT/System.pm
@@ -117,28 +117,46 @@ This method as well returns rights of other RT objects,
like L<RT::Queue> or L<RT::Group>. To allow users to apply
those rights globally.
+If an L<RT::Principal> is passed as the first argument, the available rights
+will be limited to ones which make sense for the principal. Currently only
+role groups are supported and rights announced by object types to which the
+role group doesn't apply are not returned.
+
=cut
sub AvailableRights {
- my $self = shift;
+ my $self = shift;
+ my $principal = shift;
+ my @types = keys %RT::ACE::OBJECT_TYPES;
+
+ # Include global system rights by default
+ my %rights = %{ $RIGHTS };
+
+ # Only return rights on classes which support the role asked for
+ if ($principal and $principal->IsRoleGroup) {
+ my $role = $principal->Object->Type;
+ @types = grep { $_->HasRole($role) } @types;
+ %rights = ();
+ }
- # Build a merged list of all system wide rights, queue rights, group rights, etc.
- my %rights = (
- %{ $RIGHTS },
- %{ $self->_ForAllACEObjectTypes('AvailableRights') },
+ # Build a merged list of system wide rights, queue rights, group rights, etc.
+ %rights = (
+ %rights,
+ %{ $self->_ForACEObjectTypes(\@types => 'AvailableRights', @_) },
);
delete $rights{ExecuteCode} if RT->Config->Get('DisallowExecuteCode');
return(\%rights);
}
-sub _ForAllACEObjectTypes {
- my $self = shift;
+sub _ForACEObjectTypes {
+ my $self = shift;
+ my $types = shift || [];
my $method = shift;
- return {} unless $method;
+ return {} unless @$types and $method;
my %data;
- for my $class (sort keys %RT::ACE::OBJECT_TYPES) {
+ for my $class (sort @$types) {
next unless $RT::ACE::OBJECT_TYPES{$class};
# Skip ourselves otherwise we'd loop infinitely
@@ -154,7 +172,7 @@ sub _ForAllACEObjectTypes {
# embrace and extend
%data = (
%data,
- %{ $object->$method || {} },
+ %{ $object->$method(@_) || {} },
);
}
@@ -174,7 +192,7 @@ sub RightCategories {
# Build a merged list of all right categories system wide, per-queue, per-group, etc.
my %categories = (
%{ $RIGHT_CATEGORIES },
- %{ $self->_ForAllACEObjectTypes('RightCategories') },
+ %{ $self->_ForACEObjectTypes([keys %RT::ACE::OBJECT_TYPES] => 'RightCategories') },
);
return \%categories;
diff --git a/share/html/Admin/Elements/EditRightsCategoryTabs b/share/html/Admin/Elements/EditRightsCategoryTabs
index 786cafd..ae10d90 100644
--- a/share/html/Admin/Elements/EditRightsCategoryTabs
+++ b/share/html/Admin/Elements/EditRightsCategoryTabs
@@ -52,15 +52,10 @@ $id
$acldesc => ''
</%args>
<%init>
-# XXX OPTIMIZATION: Moving the calls to AvailableRights and RightCategories up
-# one component to avoid calling them for every principal would be a win, but
-# it's cleaner to do it here. The values can really be computed once per
-# $Context.
-
# Find all our available rights...
my (%available_rights, %categories);
if ( blessed($Context) and $Context->can('AvailableRights') ) {
- %available_rights = %{$Context->AvailableRights};
+ %available_rights = %{$Context->AvailableRights( $Principal ? $Principal->PrincipalObj : undef )};
} else {
%available_rights = ( loc('System Error') => loc("No rights found") );
}
commit 4ceffb95dc789d350c285d01d1d0f2092021cd8f
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Sep 20 12:37:04 2012 -0700
Basic tests for variable AvailableRights
Adds a new dep for testing, Set::Tiny, which is currently used for a
single test. The expectation is it will be used more in the future,
hence the inclusion as a dep.
diff --git a/sbin/rt-test-dependencies.in b/sbin/rt-test-dependencies.in
index 56d5db2..54efc23 100644
--- a/sbin/rt-test-dependencies.in
+++ b/sbin/rt-test-dependencies.in
@@ -300,6 +300,7 @@ Test::WWW::Mechanize::PSGI
Plack::Middleware::Test::StashWarnings 0.06
Test::LongString
Mojo::DOM
+Set::Tiny
.
$deps{'FASTCGI'} = [ text_to_hash( << '.') ];
diff --git a/t/api/system-available-rights.t b/t/api/system-available-rights.t
new file mode 100644
index 0000000..9c374d6
--- /dev/null
+++ b/t/api/system-available-rights.t
@@ -0,0 +1,65 @@
+use strict;
+use warnings;
+
+use RT::Test tests => undef;
+use Set::Tiny;
+
+my @warnings;
+local $SIG{__WARN__} = sub {
+ push @warnings, "@_";
+};
+
+my $requestor = RT::Group->new( RT->SystemUser );
+$requestor->LoadRoleGroup(
+ Object => RT->System,
+ Type => "Requestor",
+);
+ok $requestor->id, "Loaded global requestor role group";
+
+$requestor = $requestor->PrincipalObj;
+ok $requestor->id, "Loaded global requestor role group principal";
+
+note "Try granting an article right to a system role group";
+{
+ my ($ok, $msg) = $requestor->GrantRight(
+ Right => "ShowArticle",
+ Object => RT->System,
+ );
+ ok !$ok, "Couldn't grant nonsensical right to global Requestor role: $msg";
+ like shift @warnings, qr/Couldn't validate right name.*?ShowArticle/;
+
+ ($ok, $msg) = $requestor->GrantRight(
+ Right => "ShowTicket",
+ Object => RT->System,
+ );
+ ok $ok, "Granted queue right to global queue role: $msg";
+
+ ($ok, $msg) = RT->PrivilegedUsers->PrincipalObj->GrantRight(
+ Right => "ShowArticle",
+ Object => RT->System,
+ );
+ ok $ok, "Granted article right to non-role global group: $msg";
+
+ reset_rights();
+}
+
+note "AvailableRights";
+{
+ my @available = (
+ [ keys %{RT->System->AvailableRights} ],
+ [ keys %{RT->System->AvailableRights( $requestor )} ],
+ );
+
+ my $all = Set::Tiny->new( @{$available[0]} );
+ my $role = Set::Tiny->new( @{$available[1]} );
+
+ ok $role->is_proper_subset($all), "role rights are a proper subset of all";
+}
+
+ok !@warnings, "No uncaught warnings"
+ or diag explain \@warnings;
+
+# for clarity
+sub reset_rights { RT::Test->set_rights() }
+
+done_testing;
commit eaec506de29d8ef19b76b940a49c627cbc137121
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Sep 20 13:01:58 2012 -0700
Move unused role data structure used for l10n closer to role registration
The variable is no longer a package variable since we have an API.
diff --git a/lib/RT/ACE.pm b/lib/RT/ACE.pm
index 914670d..5cbe7de 100644
--- a/lib/RT/ACE.pm
+++ b/lib/RT/ACE.pm
@@ -78,7 +78,6 @@ use RT::Groups;
use vars qw (
%LOWERCASERIGHTNAMES
%OBJECT_TYPES
- %TICKET_METAPRINCIPALS
);
@@ -90,21 +89,6 @@ use vars qw (
=cut
-
-
-
-
-
-%TICKET_METAPRINCIPALS = (
- Owner => 'The owner of a ticket', # loc_pair
- Requestor => 'The requestor of a ticket', # loc_pair
- Cc => 'The CC of a ticket', # loc_pair
- AdminCc => 'The administrative CC of a ticket', # loc_pair
-);
-
-
-
-
=head2 LoadByValues PARAMHASH
Load an ACE by specifying a paramhash with the following fields:
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index e89d4b8..6c102fd 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -84,7 +84,15 @@ use RT::URI;
use MIME::Entity;
use Devel::GlobalDestruction;
-for my $role (qw(Requestor Cc AdminCc Owner)) {
+my %ROLES = (
+ # name => description
+ Owner => 'The owner of a ticket', # loc_pair
+ Requestor => 'The requestor of a ticket', # loc_pair
+ Cc => 'The CC of a ticket', # loc_pair
+ AdminCc => 'The administrative CC of a ticket', # loc_pair
+);
+
+for my $role (sort keys %ROLES) {
RT::Ticket->RegisterRole(
Name => $role,
EquivClasses => ['RT::Queue'],
commit 42febb092c5ae2fdc40327b63806146c16fd5440
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Sep 21 12:59:12 2012 -0700
Remove nonsensical rights granted to system role groups
These rights are invalidated by 07e7ce9 and no longer displayed in the
rights editor. Existing installations which previously granted the
invalid rights would see a slew of "Right removed" messages when the
rights editor pages were submited, even though the user hadn't actively
removed any rights. To avoid confusion, and clean up the database,
preemptively remove the useless rights during upgrade.
The set of removed rights for each role is the difference between the set of
global rights (RT->System->AvailableRights) and the set of role rights
(RT->System->AvailableRights($PrincipalObjectForSystemRoleGroup)).
diff --git a/etc/upgrade/4.1.4/content b/etc/upgrade/4.1.4/content
new file mode 100644
index 0000000..b3f59f6
--- /dev/null
+++ b/etc/upgrade/4.1.4/content
@@ -0,0 +1,49 @@
+use strict;
+use warnings;
+
+our (@Final);
+
+push @Final, sub {
+ my %global = %{ RT->System->AvailableRights };
+ my $handle = RT->DatabaseHandle;
+
+ for my $role (RT::System->Roles) {
+ my $group = RT::Group->new( RT->SystemUser );
+ my ($ok, $msg) = $group->LoadRoleGroup(
+ Object => RT->System,
+ Type => $role,
+ );
+
+ unless ($group->id) {
+ RT->Logger->error("Can't load role group $role: $msg");
+ next;
+ }
+
+ my %rights = %{ RT->System->AvailableRights( $group->PrincipalObj ) };
+
+ # Global rights which aren't available on the role anymore
+ my @remove = grep { not $rights{$_} }
+ keys %global;
+ my $placeholders = join ",", map { "?" } 1 .. scalar @remove;
+
+ my $query = <<" SQL";
+ DELETE FROM ACL
+ WHERE PrincipalType = ?
+ AND PrincipalId = ?
+ AND ObjectType = 'RT::System'
+ AND RightName IN ($placeholders)
+ SQL
+
+ my $res = $handle->SimpleQuery(
+ $query,
+ $role, # Type
+ $group->PrincipalId, # Id
+ @remove, # Right names
+ );
+
+ unless ($res) {
+ RT->Logger->error("Failed to delete invalid rights on system role $role!");
+ next;
+ }
+ }
+};
commit d659343c0e6bbba26c116735ac185f88d3f02f5f
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Sep 21 16:51:42 2012 -0700
Convenience method for loading role groups on records
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 3a22d77..1391d7e 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2124,6 +2124,28 @@ sub HasRole {
return scalar grep { $type eq $_ } $self->Roles;
}
+=head2 RoleGroup
+
+Expects a role name as the first parameter which is used to load the
+L<RT::Group> for the specified role on this record. Returns an unloaded
+L<RT::Group> object on failure.
+
+=cut
+
+sub RoleGroup {
+ my $self = shift;
+ my $type = shift;
+ my $group = RT::Group->new( $self->CurrentUser );
+
+ if ($self->HasRole($type)) {
+ $group->LoadRoleGroup(
+ Object => $self,
+ Type => $type,
+ );
+ }
+ return $group;
+}
+
RT::Base->_ImportOverlays();
1;
commit dde7d91700593e60f7ece620e095e37c3b8933bb
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Sep 26 15:40:38 2012 -0700
Convenience methods to add/delete members from record role groups
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 1391d7e..d357455 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2146,6 +2146,99 @@ sub RoleGroup {
return $group;
}
+=head2 AddRoleMember
+
+Adds the described L<RT::Principal> to the specified role group for this record.
+
+Takes a set of key-value pairs:
+
+=over 4
+
+=item PrincipalId
+
+Optional. The ID of the L<RT::Principal> object to add.
+
+=item User
+
+=item Group
+
+Optional. The Name of an L<RT::User> or L<RT::Group>, respectively, to use as
+the principal.
+
+=item Type
+
+Required. One of the valid roles for this record, as returned by L</Roles>.
+
+=back
+
+One, and only one, of I<PrincipalId>, I<User>, or I<Group> is required.
+
+Returns a tuple of (status, message).
+
+=cut
+
+sub AddRoleMember {
+ my $self = shift;
+ my %args = (@_);
+
+ return (0, $self->loc("One, and only one, of PrincipalId/User/Group is required"))
+ if 1 != grep { $_ } @args{qw/PrincipalId User Group/};
+
+ return (0, $self->loc("No valid Type specified"))
+ unless $args{Type} and $self->HasRole($args{Type});
+
+ unless ($args{PrincipalId}) {
+ my $object;
+ if ($args{User}) {
+ $object = RT::User->new( $self->CurrentUser );
+ $object->Load(delete $args{User});
+ }
+ elsif ($args{Group}) {
+ $object = RT::Group->new( $self->CurrentUser );
+ $object->LoadUserDefinedGroup(delete $args{Group});
+ }
+ $args{PrincipalId} = $object->PrincipalObj->id;
+ }
+
+ return (0, $self->loc("No valid PrincipalId"))
+ unless $args{PrincipalId};
+
+ return $self->RoleGroup(delete $args{Type})->_AddMember(%args);
+}
+
+=head2 DeleteRoleMember
+
+Removes the specified L<RT::Principal> from the specified role group for this
+record.
+
+Takes a set of key-value pairs:
+
+=over 4
+
+=item PrincipalId
+
+Required. The ID of the L<RT::Principal> object to remove.
+
+=item Type
+
+Required. One of the valid roles for this record, as returned by L</Roles>.
+
+=back
+
+Returns a tuple of (status, message).
+
+=cut
+
+sub DeleteRoleMember {
+ my $self = shift;
+ my %args = (@_);
+
+ return (0, $self->loc("No valid Type specified"))
+ unless $args{Type} and $self->HasRole($args{Type});
+
+ return $self->RoleGroup($args{Type})->_DeleteMember(delete $args{PrincipalId});
+}
+
RT::Base->_ImportOverlays();
1;
commit 71d118b7c4f110560dd4c6d2f08e0638e581e285
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Nov 8 17:06:01 2012 -0800
Record a txn on the RT::Record object when adding or removing a role member
A transaction is already recorded on the RT::Group itself, but it's
useful to have a transaction directly on the RT::Record too.
Doing this in the generic methods is easier than subclass-by-subclass
since AddRoleMember already validates PrincipalId and translates from
acceptable User/Group arguments. Subclasses which don't want to record
a transaction can simply wrap the methods to force Silent => 1.
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index d357455..b2e2e1a 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2184,8 +2184,9 @@ sub AddRoleMember {
return (0, $self->loc("One, and only one, of PrincipalId/User/Group is required"))
if 1 != grep { $_ } @args{qw/PrincipalId User Group/};
+ my $type = delete $args{Type};
return (0, $self->loc("No valid Type specified"))
- unless $args{Type} and $self->HasRole($args{Type});
+ unless $type and $self->HasRole($type);
unless ($args{PrincipalId}) {
my $object;
@@ -2203,7 +2204,17 @@ sub AddRoleMember {
return (0, $self->loc("No valid PrincipalId"))
unless $args{PrincipalId};
- return $self->RoleGroup(delete $args{Type})->_AddMember(%args);
+ my ($ok, $msg) = $self->RoleGroup($type)->_AddMember(%args);
+
+ if ($ok and not $args{Silent}) {
+ $self->_NewTransaction(
+ Type => 'AddWatcher', # use "watcher" for history's sake
+ NewValue => $args{PrincipalId},
+ Field => $type,
+ );
+ }
+
+ return ($ok, $msg);
}
=head2 DeleteRoleMember
@@ -2236,7 +2247,16 @@ sub DeleteRoleMember {
return (0, $self->loc("No valid Type specified"))
unless $args{Type} and $self->HasRole($args{Type});
- return $self->RoleGroup($args{Type})->_DeleteMember(delete $args{PrincipalId});
+ my ($ok, $msg) = $self->RoleGroup($args{Type})->_DeleteMember($args{PrincipalId});
+
+ if ($ok and not $args{Silent}) {
+ $self->_NewTransaction(
+ Type => 'DelWatcher', # use "watcher" for history's sake
+ OldValue => $args{PrincipalId},
+ Field => $args{Type},
+ );
+ }
+ return ($ok, $msg);
}
RT::Base->_ImportOverlays();
commit 6c0e66896053ea108adae6ac7853557b24d8d2c7
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Nov 8 17:08:56 2012 -0800
Validate that PrincipalId is actually passed to DeleteRoleMember, as doc'd
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index b2e2e1a..b2a2a1d 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2247,6 +2247,9 @@ sub DeleteRoleMember {
return (0, $self->loc("No valid Type specified"))
unless $args{Type} and $self->HasRole($args{Type});
+ return (0, $self->loc("No valid PrincipalId"))
+ unless $args{PrincipalId};
+
my ($ok, $msg) = $self->RoleGroup($args{Type})->_DeleteMember($args{PrincipalId});
if ($ok and not $args{Silent}) {
commit 89be28f4395493952d4b73b026a2f1946a14fe82
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Nov 26 17:35:04 2012 -0500
Refactor DeferOwner logic
The original phrasing confusingly calls ->OwnerGroup->_AddMember even if
the user does not have OwnTicket. However, $Owner is still set to
Nobody in this case; clean up the logic to make this path clearer.
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 6c102fd..0cfb0ae 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -615,27 +615,32 @@ sub Create {
}
# }}}
- # Now that we've created the ticket and set up its metadata, we can actually go and check OwnTicket on the ticket itself.
- # This might be different than before in cases where extensions like RTIR are doing clever things with RT's ACL system
- if ( $DeferOwner ) {
- if (!$DeferOwner->HasRight( Object => $self, Right => 'OwnTicket')) {
-
- $RT::Logger->warning( "User " . $DeferOwner->Name . "(" . $DeferOwner->id
+
+
+ # Now that we've created the ticket and set up its metadata, we can
+ # actually go and check OwnTicket on the ticket itself. This might
+ # be different than before in cases where extensions like RTIR are
+ # doing clever things with RT's ACL system.
+ if ($DeferOwner) {
+ if ( $DeferOwner->HasRight( Object => $self, Right => 'OwnTicket' ) ) {
+ $self->__Set( Field => 'Owner', Value => $DeferOwner->id );
+ $self->OwnerGroup->_AddMember(
+ PrincipalId => $DeferOwner->PrincipalId,
+ InsideTransaction => 1,
+ );
+ } else {
+ $self->OwnerGroup->_AddMember(
+ PrincipalId => RT->Nobody->PrincipalId,
+ InsideTransaction => 1,
+ );
+ $RT::Logger->warning( "User " . $DeferOwner->Name . "(" . $DeferOwner->id
. ") was proposed as a ticket owner but has no rights to own "
. "tickets in " . $QueueObj->Name );
push @non_fatal_errors, $self->loc(
"Owner '[_1]' does not have rights to own this ticket.",
$DeferOwner->Name
);
- } else {
- $Owner = $DeferOwner;
- $self->__Set(Field => 'Owner', Value => $Owner->id);
-
}
- $self->OwnerGroup->_AddMember(
- PrincipalId => $Owner->PrincipalId,
- InsideTransaction => 1
- );
}
if ( $args{'_RecordTransaction'} ) {
diff --git a/t/ticket/deferred_owner.t b/t/ticket/deferred_owner.t
index fe90d53..b30a794 100644
--- a/t/ticket/deferred_owner.t
+++ b/t/ticket/deferred_owner.t
@@ -1,10 +1,7 @@
-
use strict;
use warnings;
-use RT::Test nodata => 1, tests => 18;
-use_ok('RT');
-use_ok('RT::Ticket');
+use RT::Test nodata => 1, tests => undef;
use Test::Warn;
@@ -63,7 +60,7 @@ diag "check that previous trick doesn't work without sufficient rights";
diag $msg if $msg;
ok $tid, "created a ticket";
is $ticket->Owner, $tester->id, 'correct owner';
- unlike $ticket->AdminCcAddresses, qr/root\@localhost/, 'root is there';
+ unlike $ticket->AdminCcAddresses, qr/root\@localhost/, 'root is not there';
}
diag "check that deffering owner really works";
@@ -88,6 +85,9 @@ diag "check that deffering owner really works";
ok $tid, "created a ticket";
like $ticket->CcAddresses, qr/tester\@localhost/, 'tester is in the cc list';
is $ticket->Owner, $tester->id, 'tester is also owner';
+ my $owners = $ticket->OwnerGroup->MembersObj;
+ is $owners->Count, 1, 'one record in owner group';
+ is $owners->First->MemberObj->Id, $tester->id, 'and it is tester';
}
diag "check that deffering doesn't work without correct rights";
@@ -112,8 +112,10 @@ diag "check that deffering doesn't work without correct rights";
diag $msg if $msg;
ok $tid, "created a ticket";
like $ticket->CcAddresses, qr/tester\@localhost/, 'tester is in the cc list';
- isnt $ticket->Owner, $tester->id, 'tester is also owner';
+ is $ticket->Owner, RT->Nobody->id, 'nobody is the owner';
+ my $owners = $ticket->OwnerGroup->MembersObj;
+ is $owners->Count, 1, 'one record in owner group';
+ is $owners->First->MemberObj->Id, RT->Nobody->id, 'and it is nobody';
}
-
-
+done_testing;
commit dd7fbf03f5bd890cde6e90f83b07a30de13d2213
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Nov 26 17:38:10 2012 -0500
Stop splitting SetOwner into _Set and _NewTransaction
This split, along with the comment that goes with it, originated in
4137c44, where the ->Commit statement occurred between two calls to
->_Set, one which changed the record, the other which added a
Transaction. This was due to scrips, at the time, needing to be
triggered outside of a database transaction, due to the lack of nested
transactions.
Long after this restriction had been lifted, 6061508 moved ->Commit to
the more sensible place at the end of the updates, but left the two
calls to _Set (one of which had changed into _NewTransaction in e04e4a0)
in place, along with the misleading comment.
Replace both calls with one straightforward call to ->_Set.
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 0cfb0ae..e8e12be 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -2815,16 +2815,10 @@ sub SetOwner {
return ( 0, $self->loc("Could not change owner: [_1]", $add_msg ) );
}
- # We call set twice with slightly different arguments, so
- # as to not have an SQL transaction span two RT transactions
-
( $val, $msg ) = $self->_Set(
- Field => 'Owner',
- RecordTransaction => 0,
- Value => $NewOwnerObj->Id,
- TimeTaken => 0,
- TransactionType => 'Set',
- CheckACL => 0, # don't check acl
+ Field => 'Owner',
+ Value => $NewOwnerObj->Id,
+ CheckACL => 0, # don't check acl
);
unless ($val) {
@@ -2832,22 +2826,8 @@ sub SetOwner {
return ( 0, $self->loc("Could not change owner: [_1]", $msg) );
}
- ($val, $msg) = $self->_NewTransaction(
- Type => 'Set',
- Field => 'Owner',
- NewValue => $NewOwnerObj->Id,
- OldValue => $OldOwnerObj->Id,
- TimeTaken => 0,
- );
-
- if ( $val ) {
- $msg = $self->loc( "Owner changed from [_1] to [_2]",
- $OldOwnerObj->Name, $NewOwnerObj->Name );
- }
- else {
- $RT::Handle->Rollback();
- return ( 0, $msg );
- }
+ $msg = $self->loc( "Owner changed from [_1] to [_2]",
+ $OldOwnerObj->Name, $NewOwnerObj->Name );
$RT::Handle->Commit();
commit d2be0827d2619585b1a8cdbaabd69a3ebeaedde2
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Nov 26 18:32:03 2012 -0500
Remove UpdateTicket argument to _Set, simplifying logic
The last existing call site which used UpdateTicket => 0 was removed in
e04e4a0 -- which was the call that UpdateTicket's functionality was
originally added for in 4137c44.
Remove the argument, and simplify the logic of _Set considerably. The
equivalent functionality can be accomplished via _NewTransaction, which
is what other codepaths do when similar effects are desired.
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index e8e12be..d38675a 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -3313,57 +3313,42 @@ sub _Set {
Value => undef,
TimeTaken => 0,
RecordTransaction => 1,
- UpdateTicket => 1,
CheckACL => 1,
TransactionType => 'Set',
@_ );
if ($args{'CheckACL'}) {
- unless ( $self->CurrentUserHasRight('ModifyTicket')) {
- return ( 0, $self->loc("Permission Denied"));
- }
- }
-
- unless ($args{'UpdateTicket'} || $args{'RecordTransaction'}) {
- $RT::Logger->error("Ticket->_Set called without a mandate to record an update or update the ticket");
- return(0, $self->loc("Internal Error"));
+ unless ( $self->CurrentUserHasRight('ModifyTicket')) {
+ return ( 0, $self->loc("Permission Denied"));
+ }
}
- #if the user is trying to modify the record
+ # Avoid ACL loops using _Value
+ my $Old = $self->SUPER::_Value($args{'Field'});
- #Take care of the old value we really don't want to get in an ACL loop.
- # so ask the super::_Value
- my $Old = $self->SUPER::_Value("$args{'Field'}");
-
- my ($ret, $msg);
- if ( $args{'UpdateTicket'} ) {
+ # Set the new value
+ my ( $ret, $msg ) = $self->SUPER::_Set(
+ Field => $args{'Field'},
+ Value => $args{'Value'}
+ );
+ return ( 0, $msg ) unless $ret;
- #Set the new value
- ( $ret, $msg ) = $self->SUPER::_Set( Field => $args{'Field'},
- Value => $args{'Value'} );
-
- #If we can't actually set the field to the value, don't record
- # a transaction. instead, get out of here.
- return ( 0, $msg ) unless $ret;
- }
+ return ( $ret, $msg ) unless $args{'RecordTransaction'};
- if ( $args{'RecordTransaction'} == 1 ) {
+ my $trans;
+ ( $ret, $msg, $trans ) = $self->_NewTransaction(
+ Type => $args{'TransactionType'},
+ Field => $args{'Field'},
+ NewValue => $args{'Value'},
+ OldValue => $Old,
+ TimeTaken => $args{'TimeTaken'},
+ );
- my ( $Trans, $Msg, $TransObj ) = $self->_NewTransaction(
- Type => $args{'TransactionType'},
- Field => $args{'Field'},
- NewValue => $args{'Value'},
- OldValue => $Old,
- TimeTaken => $args{'TimeTaken'},
- );
- # Ensure that we can read the transaction, even if the change
- # just made the ticket unreadable to us
- $TransObj->{ _object_is_readable } = 1;
- return ( $Trans, scalar $TransObj->BriefDescription );
- }
- else {
- return ( $ret, $msg );
- }
+ # Ensure that we can read the transaction, even if the change
+ # just made the ticket unreadable to us
+ $trans->{ _object_is_readable } = 1;
+
+ return ( $ret, scalar $trans->BriefDescription );
}
commit 4fab867b656225d9aede25e63e801ae890429d78
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Nov 26 20:29:23 2012 -0500
Deprecate and move away from RT::Group->Load____RoleGroup methods
Use the generalized and shorter $obj->RoleGroup( '...' ) form instead,
or the ->LoadRoleGroup( Object => $obj, Type => '...' ) method if that
is not applicable.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index f5548ed..8fada3e 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -339,71 +339,76 @@ sub LoadRoleGroup {
=head2 LoadTicketRoleGroup { Ticket => TICKET_ID, Type => TYPE }
-Loads a ticket group from the database.
-
-Takes a param hash with 2 parameters:
-
- Ticket is the TicketId we're curious about
- Type is the type of Group we're trying to load:
- Requestor, Cc, AdminCc, Owner
+Deprecated in favor of L</LoadRoleGroup> or L<RT::Record/RoleGroup>.
=cut
sub LoadTicketRoleGroup {
- my $self = shift;
- my %args = (Ticket => '0',
- Type => undef,
- @_);
- $self->LoadByCols( Domain => 'RT::Ticket-Role',
- Instance =>$args{'Ticket'},
- Type => $args{'Type'}
- );
+ my $self = shift;
+ my %args = (
+ Ticket => '0',
+ Type => undef,
+ @_,
+ );
+ RT->Logger->warn(<<" .");
+RT::Group->LoadTicketRoleGroup is DEPRECATED and will be removed in a future release.
+
+Please use RT::Group->LoadRoleGroup or RT::Ticket->RoleGroup instead at @{[join '/', caller]}.
+ .
+ $self->LoadByCols(
+ Domain => 'RT::Ticket-Role',
+ Instance => $args{'Ticket'},
+ Type => $args{'Type'},
+ );
}
=head2 LoadQueueRoleGroup { Queue => Queue_ID, Type => TYPE }
-Loads a Queue group from the database.
-
-Takes a param hash with 2 parameters:
-
- Queue is the QueueId we're curious about
- Type is the type of Group we're trying to load:
- Requestor, Cc, AdminCc, Owner
+Deprecated in favor of L</LoadRoleGroup> or L<RT::Record/RoleGroup>.
=cut
sub LoadQueueRoleGroup {
- my $self = shift;
- my %args = (Queue => undef,
- Type => undef,
- @_);
- $self->LoadByCols( Domain => 'RT::Queue-Role',
- Instance =>$args{'Queue'},
- Type => $args{'Type'}
- );
+ my $self = shift;
+ my %args = (
+ Queue => undef,
+ Type => undef,
+ @_,
+ );
+ RT->Logger->warn(<<" .");
+RT::Group->LoadQueueRoleGroup is DEPRECATED and will be removed in a future release.
+
+Please use RT::Group->LoadRoleGroup or RT::Queue->RoleGroup instead at @{[join '/', caller]}.
+ .
+ $self->LoadByCols(
+ Domain => 'RT::Queue-Role',
+ Instance => $args{'Queue'},
+ Type => $args{'Type'},
+ );
}
=head2 LoadSystemRoleGroup Type
-Loads a System group from the database.
-
-Takes a single param: Type
-
- Type is the type of Group we're trying to load:
- Requestor, Cc, AdminCc, Owner
+Deprecated in favor of L</LoadRoleGroup> or L<RT::Record/RoleGroup>.
=cut
sub LoadSystemRoleGroup {
- my $self = shift;
+ my $self = shift;
my $type = shift;
- $self->LoadByCols( Domain => 'RT::System-Role',
- Type => $type
- );
+ RT->Logger->warn(<<" .");
+RT::Group->LoadSystemRoleGroup is DEPRECATED and will be removed in a future release.
+
+Please use RT::Group->LoadRoleGroup or RT::System->RoleGroup instead at @{[join '/', caller]}.
+ .
+ $self->LoadByCols(
+ Domain => 'RT::System-Role',
+ Type => $type
+ );
}
diff --git a/lib/RT/Handle.pm b/lib/RT/Handle.pm
index 2b57d0b..0f57476 100644
--- a/lib/RT/Handle.pm
+++ b/lib/RT/Handle.pm
@@ -707,8 +707,7 @@ sub InsertInitialData {
# system role groups
foreach my $name (qw(Owner Requestor Cc AdminCc)) {
- my $group = RT::Group->new( RT->SystemUser );
- $group->LoadSystemRoleGroup( $name );
+ my $group = RT->System->RoleGroup( $name );
if ( $group->id ) {
push @warns, "System role '$name' already exists.";
next;
@@ -966,12 +965,11 @@ sub InsertData {
} elsif ( $item->{'GroupDomain'} eq 'SystemInternal' ) {
$princ->LoadSystemInternalGroup( $item->{'GroupType'} );
} elsif ( $item->{'GroupDomain'} eq 'RT::System-Role' ) {
- $princ->LoadSystemRoleGroup( $item->{'GroupType'} );
+ $princ->LoadRoleGroup( Object => RT->System, Type => $item->{'GroupType'} );
} elsif ( $item->{'GroupDomain'} eq 'RT::Queue-Role' &&
$item->{'Queue'} )
{
- $princ->LoadQueueRoleGroup( Type => $item->{'GroupType'},
- Queue => $object->id);
+ $princ->LoadRoleGroup( Object => $object, Type => $item->{'GroupType'} );
} else {
$princ->Load( $item->{'GroupId'} );
}
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 4bcaed5..125530f 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -969,8 +969,7 @@ sub _AddWatcher {
return(0, $self->loc("Could not find or create that user"));
}
- my $group = RT::Group->new($self->CurrentUser);
- $group->LoadQueueRoleGroup(Type => $args{'Type'}, Queue => $self->Id);
+ my $group = $self->RoleGroup( $args{'Type'} );
unless ($group->id) {
return(0,$self->loc("Group not found"));
}
@@ -1044,8 +1043,7 @@ sub DeleteWatcher {
return ( 0, $self->loc("Could not find that principal") );
}
- my $group = RT::Group->new($self->CurrentUser);
- $group->LoadQueueRoleGroup(Type => $args{'Type'}, Queue => $self->Id);
+ my $group = $self->RoleGroup( $args{'Type'} );
unless ($group->id) {
return(0,$self->loc("Group not found"));
}
@@ -1126,12 +1124,9 @@ If the user doesn't have "ShowQueue" permission, returns an empty group
sub Cc {
my $self = shift;
- my $group = RT::Group->new($self->CurrentUser);
- if ( $self->CurrentUserHasRight('SeeQueue') ) {
- $group->LoadQueueRoleGroup(Type => 'Cc', Queue => $self->Id);
- }
- return ($group);
-
+ return RT::Group->new($self->CurrentUser)
+ unless $self->CurrentUserHasRight('SeeQueue');
+ return $self->RoleGroup( 'Cc' );
}
@@ -1147,12 +1142,9 @@ If the user doesn't have "ShowQueue" permission, returns an empty group
sub AdminCc {
my $self = shift;
- my $group = RT::Group->new($self->CurrentUser);
- if ( $self->CurrentUserHasRight('SeeQueue') ) {
- $group->LoadQueueRoleGroup(Type => 'AdminCc', Queue => $self->Id);
- }
- return ($group);
-
+ return RT::Group->new($self->CurrentUser)
+ unless $self->CurrentUserHasRight('SeeQueue');
+ return $self->RoleGroup( 'AdminCc' );
}
@@ -1180,9 +1172,8 @@ sub IsWatcher {
@_
);
- # Load the relevant group.
- my $group = RT::Group->new($self->CurrentUser);
- $group->LoadQueueRoleGroup(Type => $args{'Type'}, Queue => $self->id);
+ # Load the relevant group.
+ my $group = $self->RoleGroup( $args{'Type'} );
# Ask if it has the member in question
my $principal = RT::Principal->new($self->CurrentUser);
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index d38675a..a8dab01 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -990,9 +990,7 @@ A constructor which returns an RT::Group object containing the owner of this tic
sub OwnerGroup {
my $self = shift;
- my $owner_obj = RT::Group->new($self->CurrentUser);
- $owner_obj->LoadTicketRoleGroup( Ticket => $self->Id, Type => 'Owner');
- return ($owner_obj);
+ return $self->RoleGroup( 'Owner' );
}
@@ -1104,8 +1102,7 @@ sub _AddWatcher {
}
- my $group = RT::Group->new($self->CurrentUser);
- $group->LoadTicketRoleGroup(Type => $args{'Type'}, Ticket => $self->Id);
+ my $group = $self->RoleGroup( $args{'Type'} );
unless ($group->id) {
return(0,$self->loc("Group not found"));
}
@@ -1185,8 +1182,7 @@ sub DeleteWatcher {
return ( 0, $self->loc("Could not find that principal") );
}
- my $group = RT::Group->new( $self->CurrentUser );
- $group->LoadTicketRoleGroup( Type => $args{'Type'}, Ticket => $self->Id );
+ my $group = $self->RoleGroup( $args{'Type'} );
unless ( $group->id ) {
return ( 0, $self->loc("Group not found") );
}
@@ -1390,13 +1386,9 @@ Returns this ticket's Requestors as an RT::Group object
sub Requestors {
my $self = shift;
-
- my $group = RT::Group->new($self->CurrentUser);
- if ( $self->CurrentUserHasRight('ShowTicket') ) {
- $group->LoadTicketRoleGroup(Type => 'Requestor', Ticket => $self->Id);
- }
- return ($group);
-
+ return RT::Group->new($self->CurrentUser)
+ unless $self->CurrentUserHasRight('ShowTicket');
+ return $self->RoleGroup( 'Requestor' );
}
@@ -1412,12 +1404,9 @@ If the user doesn't have "ShowTicket" permission, returns an empty group
sub Cc {
my $self = shift;
- my $group = RT::Group->new($self->CurrentUser);
- if ( $self->CurrentUserHasRight('ShowTicket') ) {
- $group->LoadTicketRoleGroup(Type => 'Cc', Ticket => $self->Id);
- }
- return ($group);
-
+ return RT::Group->new($self->CurrentUser)
+ unless $self->CurrentUserHasRight('ShowTicket');
+ return $self->RoleGroup( 'Cc' );
}
@@ -1433,12 +1422,9 @@ If the user doesn't have "ShowTicket" permission, returns an empty group
sub AdminCc {
my $self = shift;
- my $group = RT::Group->new($self->CurrentUser);
- if ( $self->CurrentUserHasRight('ShowTicket') ) {
- $group->LoadTicketRoleGroup(Type => 'AdminCc', Ticket => $self->Id);
- }
- return ($group);
-
+ return RT::Group->new($self->CurrentUser)
+ unless $self->CurrentUserHasRight('ShowTicket');
+ return $self->RoleGroup( 'AdminCc' );
}
@@ -1470,9 +1456,8 @@ sub IsWatcher {
@_
);
- # Load the relevant group.
- my $group = RT::Group->new($self->CurrentUser);
- $group->LoadTicketRoleGroup(Type => $args{'Type'}, Ticket => $self->id);
+ # Load the relevant group.
+ my $group = $self->RoleGroup( $args{'Type'} );
# Find the relevant principal.
if (!$args{PrincipalId} && $args{Email}) {
diff --git a/t/api/groups.t b/t/api/groups.t
index d2dc126..013e192 100644
--- a/t/api/groups.t
+++ b/t/api/groups.t
@@ -49,8 +49,7 @@ my $testuser = RT::User->new(RT->SystemUser);
($id,$msg) = $testuser->Create(Name => 'JustAnAdminCc');
ok ($id,$msg);
-my $global_admin_cc = RT::Group->new(RT->SystemUser);
-$global_admin_cc->LoadSystemRoleGroup('AdminCc');
+my $global_admin_cc = RT->System->RoleGroup( 'AdminCc' );
ok($global_admin_cc->id, "Found the global admincc group");
my $groups = RT::Groups->new(RT->SystemUser);
$groups->WithRight(Right => 'OwnTicket', Object => $q);
diff --git a/t/api/queue.t b/t/api/queue.t
index 07b8ed4..71efb4d 100644
--- a/t/api/queue.t
+++ b/t/api/queue.t
@@ -2,7 +2,7 @@
use strict;
use warnings;
use RT;
-use RT::Test nodata => 1, tests => 24;
+use RT::Test nodata => 1, tests => undef;
{
@@ -58,8 +58,7 @@ ok(!$id, $val);
my $Queue = RT::Queue->new(RT->SystemUser);
my ($id, $msg) = $Queue->Create(Name => "Foo");
ok ($id, "Foo $id was created");
-ok(my $group = RT::Group->new(RT->SystemUser));
-ok($group->LoadQueueRoleGroup(Queue => $id, Type=> 'Requestor'));
+ok(my $group = $Queue->RoleGroup('Requestor'));
ok ($group->Id, "Found the requestors object for this Queue");
{
@@ -79,13 +78,12 @@ ok ($Queue->IsWatcher(Type => 'Cc', PrincipalId => $bob->PrincipalId), "The Queu
"The Queue no longer has bob at fsck.com as a requestor");
}
-$group = RT::Group->new(RT->SystemUser);
-ok($group->LoadQueueRoleGroup(Queue => $id, Type=> 'Cc'));
+$group = $Queue->RoleGroup('Cc');
ok ($group->Id, "Found the cc object for this Queue");
-$group = RT::Group->new(RT->SystemUser);
-ok($group->LoadQueueRoleGroup(Queue => $id, Type=> 'AdminCc'));
+$group = $Queue->RoleGroup('AdminCc');
ok ($group->Id, "Found the AdminCc object for this Queue");
}
+done_testing;
diff --git a/t/api/rights.t b/t/api/rights.t
index 107fb2b..7b38cae 100644
--- a/t/api/rights.t
+++ b/t/api/rights.t
@@ -32,10 +32,8 @@ ok $user && $user->id, 'loaded or created user';
}
{
- my $group = RT::Group->new( RT->SystemUser );
- ok( $group->LoadQueueRoleGroup( Queue => $queue->id, Type=> 'Owner' ),
- "load queue owners role group"
- );
+ my $group = $queue->RoleGroup( 'Owner' );
+ ok( $group->Id, "load queue owners role group" );
my $ace = RT::ACE->new( RT->SystemUser );
my ($ace_id, $msg) = $group->PrincipalObj->GrantRight(
Right => 'ReplyToTicket', Object => $queue
@@ -87,10 +85,8 @@ my $ticket;
{
# Testing of EquivObjects
- my $group = RT::Group->new( RT->SystemUser );
- ok( $group->LoadQueueRoleGroup( Queue => $queue->id, Type=> 'AdminCc' ),
- "load queue AdminCc role group"
- );
+ my $group = $queue->RoleGroup( 'AdminCc' );
+ ok( $group->Id, "load queue AdminCc role group" );
my $ace = RT::ACE->new( RT->SystemUser );
my ($ace_id, $msg) = $group->PrincipalObj->GrantRight(
Right => 'ModifyTicket', Object => $queue
diff --git a/t/api/ticket.t b/t/api/ticket.t
index 53476f6..57e6183 100644
--- a/t/api/ticket.t
+++ b/t/api/ticket.t
@@ -2,7 +2,7 @@
use strict;
use warnings;
use RT;
-use RT::Test tests => 89;
+use RT::Test tests => undef;
{
@@ -115,8 +115,7 @@ my ($id, $msg) = $ticket->Create(Subject => "Foo",
Queue => '1'
);
ok ($id, "Ticket $id was created");
-ok(my $group = RT::Group->new(RT->SystemUser));
-ok($group->LoadTicketRoleGroup(Ticket => $id, Type=> 'Requestor'));
+ok(my $group = $ticket->RoleGroup('Requestor'));
ok ($group->Id, "Found the requestors object for this ticket");
ok(my $jesse = RT::User->new(RT->SystemUser), "Creating a jesse rt::user");
@@ -135,14 +134,11 @@ ok ( ($add_id, $add_msg) = $ticket->DeleteWatcher(Type =>'Requestor', Email => '
ok (!$ticket->IsWatcher(Type => 'Requestor', PrincipalId => $bob->PrincipalId), "The ticket no longer has bob at fsck.com as a requestor");
-$group = RT::Group->new(RT->SystemUser);
-ok($group->LoadTicketRoleGroup(Ticket => $id, Type=> 'Cc'));
+$group = $ticket->RoleGroup('Cc');
ok ($group->Id, "Found the cc object for this ticket");
-$group = RT::Group->new(RT->SystemUser);
-ok($group->LoadTicketRoleGroup(Ticket => $id, Type=> 'AdminCc'));
+$group = $ticket->RoleGroup('AdminCc');
ok ($group->Id, "Found the AdminCc object for this ticket");
-$group = RT::Group->new(RT->SystemUser);
-ok($group->LoadTicketRoleGroup(Ticket => $id, Type=> 'Owner'));
+$group = $ticket->RoleGroup('Owner');
ok ($group->Id, "Found the Owner object for this ticket");
ok($group->HasMember(RT->SystemUser->UserObj->PrincipalObj), "the owner group has the member 'RT_System'");
@@ -263,3 +259,4 @@ ok( $id, $msg );
}
+done_testing;
diff --git a/t/customfields/access_via_queue.t b/t/customfields/access_via_queue.t
index cca45e7..25e16cc 100644
--- a/t/customfields/access_via_queue.t
+++ b/t/customfields/access_via_queue.t
@@ -28,11 +28,8 @@ my $tester = RT::Test->load_or_create_user(
);
ok $tester && $tester->id, 'loaded or created user';
-my $cc_role = RT::Group->new( $queue->CurrentUser );
-$cc_role->LoadQueueRoleGroup( Type => 'Cc', Queue => $queue->id );
-
-my $owner_role = RT::Group->new( $queue->CurrentUser );
-$owner_role->LoadQueueRoleGroup( Type => 'Owner', Queue => $queue->id );
+my $cc_role = $queue->RoleGroup( 'Cc' );
+my $owner_role = $queue->RoleGroup( 'Owner' );
ok( RT::Test->set_rights(
{ Principal => $tester, Right => [qw(SeeQueue ShowTicket CreateTicket ReplyToTicket Watch OwnTicket TakeTicket)] },
diff --git a/t/mail/gateway.t b/t/mail/gateway.t
index 9482ffc..631dde4 100644
--- a/t/mail/gateway.t
+++ b/t/mail/gateway.t
@@ -793,8 +793,8 @@ while( my $ace = $acl->Next ) {
ok( !$user->HasRight( Right => 'ReplyToTicket', Object => $tick ), "User can't reply to ticket any more" );
-my $group = RT::Group->new( RT->SystemUser );
-ok( $group->LoadQueueRoleGroup( Queue => $qid, Type=> 'Owner' ), "load queue owners role group" );
+my $group = $queue->RoleGroup( 'Owner' );
+ok( $group->Id, "load queue owners role group" );
$ace = RT::ACE->new( RT->SystemUser );
($ace_id, $msg) = $group->PrincipalObj->GrantRight( Right => 'ReplyToTicket', Object => $queue );
ok( $ace_id, "Granted queue owners role group with ReplyToTicket right" );
diff --git a/t/ticket/deferred_owner.t b/t/ticket/deferred_owner.t
index b30a794..a0aa350 100644
--- a/t/ticket/deferred_owner.t
+++ b/t/ticket/deferred_owner.t
@@ -13,8 +13,7 @@ ok $tester && $tester->id, 'loaded or created user';
my $queue = RT::Test->load_or_create_queue( Name => 'General' );
ok $queue && $queue->id, 'loaded or created queue';
-my $owner_role_group = RT::Group->new( RT->SystemUser );
-$owner_role_group->LoadQueueRoleGroup( Type => 'Owner', Queue => $queue->id );
+my $owner_role_group = $queue->RoleGroup( 'Owner' );
ok $owner_role_group->id, 'loaded owners role group of the queue';
diag "check that deffering owner doesn't regress";
commit 0821df034ce6eb367e07eabe1974c6c1fcb7f2ec
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Nov 27 17:37:30 2012 -0500
Understand role groups like Owner which always contain exactly one user
The Owner group is different from all other role groups, in that it
always should contain exactly one member (even if that member is
Nobody). This generalizes that property, making such groups contain
Nobody during creation, and removes all previous members when adding a
new one.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 8fada3e..9c5f573 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -689,10 +689,20 @@ sub CreateRoleGroup {
return ( 0, $self->loc("Role group exists already") );
}
- return $self->_Create(
+ my ($id, $msg) = $self->_Create(
InsideTransaction => 1,
%create,
);
+
+ if ($self->SingleMemberRoleGroup) {
+ $self->_AddMember(
+ PrincipalId => RT->Nobody->Id,
+ InsideTransaction => 1,
+ RecordTransaction => 0,
+ );
+ }
+
+ return ($id, $msg);
}
=head2 ValidateRoleGroup
@@ -714,6 +724,17 @@ sub ValidateRoleGroup {
return $class->HasRole($args{Type});
}
+=head2 SingleMemberRoleGroup
+
+=cut
+
+sub SingleMemberRoleGroup {
+ my $self = shift;
+ my ($class) = $self->Domain =~ /^(.+)-Role$/;
+ return unless $class;
+ return $class->_ROLES->{$self->Type}{Single};
+}
+
=head2 Delete
Delete this object
@@ -1037,6 +1058,14 @@ sub _AddMember {
return ( 0, $self->loc("Groups can't be members of their members"));
}
+ if ($self->SingleMemberRoleGroup) {
+ # Purge all previous members
+ for my $owner (@{$self->MembersObj->ItemsArrayRef}) {
+ my ($ok, $msg) = $owner->Delete();
+ return(0, $self->loc("Couldn't remove previous member: [_1]", $msg))
+ unless $ok;
+ }
+ }
my $member_object = RT::GroupMember->new( $self->CurrentUser );
my $id = $member_object->Create(
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index a8dab01..8c4b74a 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -96,6 +96,7 @@ for my $role (sort keys %ROLES) {
RT::Ticket->RegisterRole(
Name => $role,
EquivClasses => ['RT::Queue'],
+ ( $role eq "Owner" ? ( Single => 1) : () ),
);
}
@@ -496,15 +497,11 @@ sub Create {
);
}
- # Set the owner in the Groups table
- # We denormalize it into the Ticket table too because doing otherwise would
- # kill performance, bigtime. It gets kept in lockstep thanks to the magic of transactionalization
+ # Set the owner in the Groups table.
$self->OwnerGroup->_AddMember(
PrincipalId => $Owner->PrincipalId,
- InsideTransaction => 1
- ) unless $DeferOwner;
-
-
+ InsideTransaction => 1,
+ );
# Deal with setting up watchers
@@ -629,10 +626,6 @@ sub Create {
InsideTransaction => 1,
);
} else {
- $self->OwnerGroup->_AddMember(
- PrincipalId => RT->Nobody->PrincipalId,
- InsideTransaction => 1,
- );
$RT::Logger->warning( "User " . $DeferOwner->Name . "(" . $DeferOwner->id
. ") was proposed as a ticket owner but has no rights to own "
. "tickets in " . $QueueObj->Name );
@@ -2773,31 +2766,18 @@ sub SetOwner {
}
my ( $val, $msg ) = $self->_IsProposedOwnerChangeValid( $NewOwnerObj, $Type );
- if ( !$val ) {
+ unless ($val) {
$RT::Handle->Rollback();
return ( $val, $msg );
}
- # Delete the owner in the owner group, then add a new one
- # TODO: is this safe? it's not how we really want the API to work
- # for most things, but it's fast.
- my ( $del_id, $del_msg );
- for my $owner (@{$self->OwnerGroup->MembersObj->ItemsArrayRef}) {
- ($del_id, $del_msg) = $owner->Delete();
- last unless ($del_id);
- }
-
- unless ($del_id) {
- $RT::Handle->Rollback();
- return ( 0, $self->loc("Could not change owner: [_1]", $del_msg) );
- }
-
- my ( $add_id, $add_msg ) = $self->OwnerGroup->_AddMember(
- PrincipalId => $NewOwnerObj->PrincipalId,
- InsideTransaction => 1 );
- unless ($add_id) {
+ ($val, $msg ) = $self->OwnerGroup->_AddMember(
+ PrincipalId => $NewOwnerObj->PrincipalId,
+ InsideTransaction => 1,
+ );
+ unless ($val) {
$RT::Handle->Rollback();
- return ( 0, $self->loc("Could not change owner: [_1]", $add_msg ) );
+ return ( 0, $self->loc("Could not change owner: [_1]", $msg ) );
}
( $val, $msg ) = $self->_Set(
diff --git a/t/mail/gateway.t b/t/mail/gateway.t
index 631dde4..7358468 100644
--- a/t/mail/gateway.t
+++ b/t/mail/gateway.t
@@ -639,6 +639,8 @@ EOF
close (MAIL);
is ($? >> 8, 0, "The mail gateway exited normally");
+DBIx::SearchBuilder::Record::Cachable->FlushCache;
+
$tick = RT::Ticket->new(RT->SystemUser);
$tick->Load( $id );
is( $tick->Id, $id, 'load correct ticket');
commit 167e69f8e479a12e6a17ab79726866434ce8624d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 29 01:17:51 2012 -0500
Maintain one-user invariant, even when a user is removed
If a user is removed from a single-user role group like Owner (which
does not currently occur -- only add), replace them with the Nobody
user.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 9c5f573..fdf50d8 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -1219,13 +1219,17 @@ sub _DeleteMember {
#Now that we've checked ACLs and sanity, delete the groupmember
my $val = $member_obj->Delete();
- if ($val) {
- return ( $val, $self->loc("Member deleted") );
- }
- else {
+ unless ($val) {
$RT::Logger->debug("Failed to delete group ".$self->Id." member ". $member_id);
return ( 0, $self->loc("Member not deleted" ));
}
+
+ $self->_AddMember(
+ PrincipalId => RT->Nobody->Id,
+ RecordTransaction => 0,
+ ) if $self->SingleMemberRoleGroup;
+
+ return ( $val, $self->loc("Member deleted") );
}
commit 656e4117c2d89e63e4d41339c99ce820339b4038
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Nov 27 18:29:36 2012 -0500
Move removal after addition of the new groupmember, to avoid id reuse
SQLite reuses primary ids, for example if a delete is followed by an
insert. This causes bugs during shredding, wherein a GroupMember is
removed and replaced with a Nobody record -- which re-uses the previous
member's id, and is thus mistakenly shredded.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index fdf50d8..d49829c 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -1058,14 +1058,9 @@ sub _AddMember {
return ( 0, $self->loc("Groups can't be members of their members"));
}
- if ($self->SingleMemberRoleGroup) {
- # Purge all previous members
- for my $owner (@{$self->MembersObj->ItemsArrayRef}) {
- my ($ok, $msg) = $owner->Delete();
- return(0, $self->loc("Couldn't remove previous member: [_1]", $msg))
- unless $ok;
- }
- }
+ my @purge;
+ push @purge, @{$self->MembersObj->ItemsArrayRef}
+ if $self->SingleMemberRoleGroup;
my $member_object = RT::GroupMember->new( $self->CurrentUser );
my $id = $member_object->Create(
@@ -1073,12 +1068,18 @@ sub _AddMember {
Group => $self->PrincipalObj,
InsideTransaction => $args{'InsideTransaction'}
);
- if ($id) {
- return ( 1, $self->loc("Member added: [_1]", $new_member_obj->Object->Name) );
- }
- else {
- return(0, $self->loc("Couldn't add member to group"));
+
+ return(0, $self->loc("Couldn't add member to group"))
+ unless $id;
+
+ # Purge all previous members
+ for my $owner (@purge) {
+ my ($ok, $msg) = $owner->Delete();
+ return(0, $self->loc("Couldn't remove previous member: [_1]", $msg))
+ unless $ok;
}
+
+ return ( 1, $self->loc("Member added: [_1]", $new_member_obj->Object->Name) );
}
commit ce0e962cef69f9e6c2bcb15ac01405ce77e4fce0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Nov 27 17:38:01 2012 -0500
Update denormalized "Owner" column on tickets on group change
Rather than explicitly handle the Owner column in all places which alter
the Owner role group, generalize that update into the group. It is thus
sufficient to update the role group membership, and the denormalized
column will be updated appropriately.
Though the object in question can be determined via the properties of
the role group, we pass the object currently being examined such that
the data structure in memory is updated with the new value -- otherwise,
it stores stale data, leading to inconsistent and conflicting results.
It is important to note that the ticket object is created with an empty
Owner field, which is set by the ->_AddMember call later. Failure to do
so causes inconsistency between the group membership and the field.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index d49829c..213fd50 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -668,7 +668,8 @@ sub CreateRoleGroup {
@_ );
# Translate Object to Domain + Instance
- if ( my $object = delete $args{Object} ) {
+ my $object = delete $args{Object};
+ if ( $object ) {
$args{Domain} = ref($object) . "-Role";
$args{Instance} = $object->id;
}
@@ -699,6 +700,7 @@ sub CreateRoleGroup {
PrincipalId => RT->Nobody->Id,
InsideTransaction => 1,
RecordTransaction => 0,
+ Object => $object,
);
}
@@ -735,6 +737,23 @@ sub SingleMemberRoleGroup {
return $class->_ROLES->{$self->Type}{Single};
}
+sub SingleMemberRoleGroupColumn {
+ my $self = shift;
+ my ($class) = $self->Domain =~ /^(.+)-Role$/;
+ return unless $class;
+ return unless $class->_ROLES->{$self->Type}{Class} eq $class;
+ return $class->_ROLES->{$self->Type}{Column};
+}
+
+sub RoleGroupObject {
+ my $self = shift;
+ my ($class) = $self->Domain =~ /^(.+)-Role$/;
+ return unless $class;
+ my $obj = $class->new( $self->CurrentUser );
+ $obj->Load( $self->Instance );
+ return $obj;
+}
+
=head2 Delete
Delete this object
@@ -1024,6 +1043,7 @@ sub _AddMember {
my $self = shift;
my %args = ( PrincipalId => undef,
InsideTransaction => undef,
+ RecordTransaction => 1,
@_);
my $new_member = $args{'PrincipalId'};
@@ -1079,6 +1099,19 @@ sub _AddMember {
unless $ok;
}
+ # Update the column
+ if (my $col = $self->SingleMemberRoleGroupColumn) {
+ my $obj = $args{Object} || $self->RoleGroupObject;
+ my ($ok, $msg) = $obj->_Set(
+ Field => $col,
+ Value => $new_member_obj->Id,
+ CheckACL => 0, # don't check acl
+ RecordTransaction => $args{'RecordTransaction'},
+ );
+ return (0, $self->loc("Could not update column $col: [_1]", $msg))
+ unless $ok;
+ }
+
return ( 1, $self->loc("Member added: [_1]", $new_member_obj->Object->Name) );
}
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index b2a2a1d..889b594 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2076,6 +2076,12 @@ sub RegisterRole {
);
return unless $role{Name};
+ # Keep track of the class this role came from originally
+ $role{ Class } ||= $class;
+
+ # Some groups are limited to a single user
+ $role{ Single } = 1 if $role{Column};
+
# Stash the role on ourself
$class->_ROLES->{ $role{Name} } = \%role;
diff --git a/lib/RT/Shredder/GroupMember.pm b/lib/RT/Shredder/GroupMember.pm
index 2da6cbd..2712d45 100644
--- a/lib/RT/Shredder/GroupMember.pm
+++ b/lib/RT/Shredder/GroupMember.pm
@@ -118,15 +118,7 @@ sub __DependsOn
}
my( $status, $msg ) = $group->AddMember( RT->Nobody->id );
- RT::Shredder::Exception->throw( $msg ) unless $status;
-
- my $ticket = RT::Ticket->new( $group->CurrentUser );
- $ticket->Load( $group->Instance );
- RT::Shredder::Exception->throw( "Couldn't load ticket" ) unless $ticket->id;
- ( $status, $msg ) = $ticket->_Set( Field => 'Owner',
- Value => RT->Nobody->id,
- );
RT::Shredder::Exception->throw( $msg ) unless $status;
return;
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 8c4b74a..82ce247 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -96,7 +96,7 @@ for my $role (sort keys %ROLES) {
RT::Ticket->RegisterRole(
Name => $role,
EquivClasses => ['RT::Queue'],
- ( $role eq "Owner" ? ( Single => 1) : () ),
+ ( $role eq "Owner" ? ( Column => "Owner") : () ),
);
}
@@ -432,7 +432,6 @@ sub Create {
my %params = (
Queue => $QueueObj->Id,
- Owner => $Owner->Id,
Subject => $args{'Subject'},
InitialPriority => $args{'InitialPriority'},
FinalPriority => $args{'FinalPriority'},
@@ -497,10 +496,13 @@ sub Create {
);
}
- # Set the owner in the Groups table.
+ # Set the owner group; this also sets the denormalized Owner field
+ # appropriately.
$self->OwnerGroup->_AddMember(
PrincipalId => $Owner->PrincipalId,
InsideTransaction => 1,
+ RecordTransaction => 0,
+ Object => $self,
);
# Deal with setting up watchers
@@ -620,10 +622,11 @@ sub Create {
# doing clever things with RT's ACL system.
if ($DeferOwner) {
if ( $DeferOwner->HasRight( Object => $self, Right => 'OwnTicket' ) ) {
- $self->__Set( Field => 'Owner', Value => $DeferOwner->id );
$self->OwnerGroup->_AddMember(
PrincipalId => $DeferOwner->PrincipalId,
InsideTransaction => 1,
+ RecordTransaction => 0,
+ Object => $self,
);
} else {
$RT::Logger->warning( "User " . $DeferOwner->Name . "(" . $DeferOwner->id
@@ -2774,19 +2777,9 @@ sub SetOwner {
($val, $msg ) = $self->OwnerGroup->_AddMember(
PrincipalId => $NewOwnerObj->PrincipalId,
InsideTransaction => 1,
+ Object => $self,
);
unless ($val) {
- $RT::Handle->Rollback();
- return ( 0, $self->loc("Could not change owner: [_1]", $msg ) );
- }
-
- ( $val, $msg ) = $self->_Set(
- Field => 'Owner',
- Value => $NewOwnerObj->Id,
- CheckACL => 0, # don't check acl
- );
-
- unless ($val) {
$RT::Handle->Rollback;
return ( 0, $self->loc("Could not change owner: [_1]", $msg) );
}
commit b2cb4b6568c01c0103801fa0a5b19203b16e6cfe
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 29 03:32:28 2012 -0500
Make CreateRoleGroup respect InsideTransaction
Contrary to its documentation, CreateRoleGroup did not respect the value
of InsideTransaction that was passed to it, forcing it to be enabled.
While the only call site which disabled InsideTransaction was during
database bootstrapping (during which you have bigger problems if role
group creation fails), update the code to be consistent with the
documentation.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 213fd50..dd588a0 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -665,6 +665,7 @@ sub CreateRoleGroup {
Type => undef,
Domain => undef,
Object => undef,
+ InsideTransaction => 1,
@_ );
# Translate Object to Domain + Instance
@@ -691,14 +692,14 @@ sub CreateRoleGroup {
}
my ($id, $msg) = $self->_Create(
- InsideTransaction => 1,
+ InsideTransaction => $args{InsideTransaction},
%create,
);
if ($self->SingleMemberRoleGroup) {
$self->_AddMember(
PrincipalId => RT->Nobody->Id,
- InsideTransaction => 1,
+ InsideTransaction => $args{InsideTransaction},
RecordTransaction => 0,
Object => $object,
);
commit 686bc38e8ec7c96458338e0a40bbc13c6214c94a
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 29 03:34:47 2012 -0500
Factor out role group creation for tickets and queues
Both tickets and queues must, immediately after record creation, create
all of their associated role groups. Refactor this into a common method
on RT::Record.
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 125530f..e32f958 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -424,7 +424,7 @@ sub Create {
return ( 0, $self->loc('Queue could not be created') );
}
- my $create_ret = $self->_CreateQueueGroups();
+ my $create_ret = $self->_CreateRoleGroups();
unless ($create_ret) {
$RT::Handle->Rollback();
return ( 0, $self->loc('Queue could not be created') );
@@ -794,49 +794,6 @@ sub IsManageableRoleGroupType {
}
-=head2 _CreateQueueGroups
-
-Create the ticket groups and links for this ticket.
-This routine expects to be called from Ticket->Create _inside of a transaction_
-
-It will create four groups for this ticket: Requestor, Cc, AdminCc and Owner.
-
-It will return true on success and undef on failure.
-
-
-=cut
-
-sub _CreateQueueGroups {
- my $self = shift;
-
- foreach my $type ($self->Roles) {
- my $ok = $self->_CreateQueueRoleGroup($type);
- return undef if !$ok;
- }
-
- return 1;
-}
-
-sub _CreateQueueRoleGroup {
- my $self = shift;
- my $type = shift;
-
- my $type_obj = RT::Group->new($self->CurrentUser);
- my ($id, $msg) = $type_obj->CreateRoleGroup(
- Type => $type,
- Object => $self,
- );
- unless ($id) {
- $RT::Logger->error("Couldn't create a Queue group of type '$type' for queue ".
- $self->Id.": ".$msg);
- return(undef);
- }
-
- return $id;
-}
-
-
-
# _HasModifyWatcherRight {{{
sub _HasModifyWatcherRight {
my $self = shift;
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 889b594..ba3d181 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2268,6 +2268,25 @@ sub DeleteRoleMember {
return ($ok, $msg);
}
+sub _CreateRoleGroups {
+ my $self = shift;
+ my %args = (@_);
+ for my $type ($self->Roles) {
+ my $type_obj = RT::Group->new($self->CurrentUser);
+ my ($id, $msg) = $type_obj->CreateRoleGroup(
+ Type => $type,
+ Object => $self,
+ %args,
+ );
+ unless ($id) {
+ $RT::Logger->error("Couldn't create a role group of type '$type' for ".ref($self)." ".
+ $self->Id.": ".$msg);
+ return(undef);
+ }
+ }
+ return(1);
+}
+
RT::Base->_ImportOverlays();
1;
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 82ce247..323a238 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -485,7 +485,8 @@ sub Create {
);
}
- my $create_groups_ret = $self->_CreateTicketGroups();
+ # Create (empty) role groups
+ my $create_groups_ret = $self->_CreateRoleGroups();
unless ($create_groups_ret) {
$RT::Logger->crit( "Couldn't create ticket groups for ticket "
. $self->Id
@@ -941,43 +942,6 @@ sub Import {
return ( $self->Id, $ErrStr );
}
-
-
-
-=head2 _CreateTicketGroups
-
-Create the ticket groups and links for this ticket.
-This routine expects to be called from Ticket->Create _inside of a transaction_
-
-It will create four groups for this ticket: Requestor, Cc, AdminCc and Owner.
-
-It will return true on success and undef on failure.
-
-
-=cut
-
-
-sub _CreateTicketGroups {
- my $self = shift;
-
- foreach my $type ($self->Roles) {
- my $type_obj = RT::Group->new($self->CurrentUser);
- my ($id, $msg) = $type_obj->CreateRoleGroup(
- Type => $type,
- Object => $self,
- );
- unless ($id) {
- $RT::Logger->error("Couldn't create a ticket group of type '$type' for ticket ".
- $self->Id.": ".$msg);
- return(undef);
- }
- }
- return(1);
-
-}
-
-
-
=head2 OwnerGroup
A constructor which returns an RT::Group object containing the owner of this ticket.
commit c40935edb20ddd59dbbd0f9ddb77c39eeca05409
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 29 04:04:26 2012 -0500
Factor out role group argument parsing
The code to parse Owners and Cc/AdminCc/Requestors is more generally
applicable than to tickets, and holds for any single-user role and other
role groups. Abstract this code into a method on RT::Record.
This temporarily removes the deferred ownership feature, as well as
(relatedly) hiding error messages caused by the proposed owner not
having OwnTicket. These shortcomings are addressed in the following
commit.
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index ba3d181..d38d8c6 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2268,6 +2268,69 @@ sub DeleteRoleMember {
return ($ok, $msg);
}
+sub _ResolveRoles {
+ my $self = shift;
+ my ($roles, %args) = (@_);
+
+ my @errors;
+ for my $role ($self->Roles) {
+ if ($self->_ROLES->{$role}{Single}) {
+ # Default to nobody if unspecified
+ my $value = $args{$role} || RT->Nobody;
+ if (Scalar::Util::blessed($value) and $value->isa("RT::User")) {
+ # Accept a user; it may not be loaded, which we catch below
+ $roles->{$role} = $value->PrincipalObj;
+ } else {
+ # Try loading by id, name, then email. If all fail, catch that below
+ my $user = RT::User->new( $self->CurrentUser );
+ $user->Load( $value );
+ # XXX: LoadOrCreateByEmail ?
+ $user->LoadByEmail( $value ) unless $user->id;
+ $roles->{$role} = $user->PrincipalObj;
+ }
+ unless ($roles->{$role}->id) {
+ push @errors, $self->loc("Invalid value for [_1]",loc($role));
+ $roles->{$role} = RT->Nobody->PrincipalObj unless $roles->{$role}->id;
+ }
+ # For consistency, we always return an arrayref
+ $roles->{$role} = [ $roles->{$role} ];
+ } else {
+ $roles->{$role} = [];
+ my @values = ref $args{ $role } ? @{ $args{$role} } : ($args{$role});
+ for my $value (grep {defined} @values) {
+ if ( $value =~ /^\d+$/ ) {
+ # This implicitly allows groups, if passed by id.
+ my $principal = RT::Principal->new( $self->CurrentUser );
+ my ($ok, $msg) = $principal->Load( $value );
+ if ($ok) {
+ push @{ $roles->{$role} }, $principal;
+ } else {
+ push @errors,
+ $self->loc("Couldn't load principal: [_1]", $msg);
+ }
+ } else {
+ my @addresses = RT::EmailParser->ParseEmailAddress( $value );
+ for my $address ( @addresses ) {
+ my $user = RT::User->new( RT->SystemUser );
+ my ($id, $msg) = $user->LoadOrCreateByEmail( $address );
+ if ( $id ) {
+ # Load it back as us, not as the system
+ # user, to be completely safe.
+ $user = RT::User->new( $self->CurrentUser );
+ $user->Load( $id );
+ push @{ $roles->{$role} }, $user->PrincipalObj;
+ } else {
+ push @errors,
+ $self->loc("Couldn't load or create user: [_1]", $msg);
+ }
+ }
+ }
+ }
+ }
+ }
+ return (@errors);
+}
+
sub _CreateRoleGroups {
my $self = shift;
my %args = (@_);
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 323a238..2c0f2dd 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -353,80 +353,10 @@ sub Create {
# }}}
- # Deal with setting the owner
-
- my $Owner;
- if ( ref( $args{'Owner'} ) eq 'RT::User' ) {
- if ( $args{'Owner'}->id ) {
- $Owner = $args{'Owner'};
- } else {
- $RT::Logger->error('Passed an empty RT::User for owner');
- push @non_fatal_errors,
- $self->loc("Owner could not be set.") . " ".
- $self->loc("Invalid value for [_1]",loc('owner'));
- $Owner = undef;
- }
- }
-
- #If we've been handed something else, try to load the user.
- elsif ( $args{'Owner'} ) {
- $Owner = RT::User->new( $self->CurrentUser );
- $Owner->Load( $args{'Owner'} );
- if (!$Owner->id) {
- $Owner->LoadByEmail( $args{'Owner'} )
- }
- unless ( $Owner->Id ) {
- push @non_fatal_errors,
- $self->loc("Owner could not be set.") . " "
- . $self->loc( "User '[_1]' could not be found.", $args{'Owner'} );
- $Owner = undef;
- }
- }
-
- #If we have a proposed owner and they don't have the right
- #to own a ticket, scream about it and make them not the owner
-
- my $DeferOwner;
- if ( $Owner && $Owner->Id != RT->Nobody->Id
- && !$Owner->HasRight( Object => $QueueObj, Right => 'OwnTicket' ) )
- {
- $DeferOwner = $Owner;
- $Owner = undef;
- $RT::Logger->debug('going to deffer setting owner');
-
- }
-
- #If we haven't been handed a valid owner, make it nobody.
- unless ( defined($Owner) && $Owner->Id ) {
- $Owner = RT::User->new( $self->CurrentUser );
- $Owner->Load( RT->Nobody->Id );
- }
-
- # }}}
-
-# We attempt to load or create each of the people who might have a role for this ticket
-# _outside_ the transaction, so we don't get into ticket creation races
- foreach my $type ( "Cc", "AdminCc", "Requestor" ) {
- $args{ $type } = [ $args{ $type } ] unless ref $args{ $type };
- foreach my $watcher ( splice @{ $args{$type} } ) {
- next unless $watcher;
- if ( $watcher =~ /^\d+$/ ) {
- push @{ $args{$type} }, $watcher;
- } else {
- my @addresses = RT::EmailParser->ParseEmailAddress( $watcher );
- foreach my $address( @addresses ) {
- my $user = RT::User->new( RT->SystemUser );
- my ($uid, $msg) = $user->LoadOrCreateByEmail( $address );
- unless ( $uid ) {
- push @non_fatal_errors,
- $self->loc("Couldn't load or create user: [_1]", $msg);
- } else {
- push @{ $args{$type} }, $user->id;
- }
- }
- }
- }
- }
+ # Figure out users for roles
+ my $roles = {};
+ push @non_fatal_errors, $self->_ResolveRoles( $roles, %args );
+ $args{$_} = $roles->{$_} for keys %{ $roles };
$RT::Handle->BeginTransaction();
@@ -499,12 +429,14 @@ sub Create {
# Set the owner group; this also sets the denormalized Owner field
# appropriately.
- $self->OwnerGroup->_AddMember(
- PrincipalId => $Owner->PrincipalId,
- InsideTransaction => 1,
- RecordTransaction => 0,
- Object => $self,
- );
+ if ($args{Owner}->HasRight( Object => $self, Right => 'OwnTicket' ) ) {
+ $self->OwnerGroup->_AddMember(
+ PrincipalId => $args{Owner}->Id,
+ InsideTransaction => 1,
+ RecordTransaction => 0,
+ Object => $self,
+ );
+ }
# Deal with setting up watchers
@@ -520,7 +452,7 @@ sub Create {
my ($val, $msg) = $self->$method(
Type => $type,
- PrincipalId => $watcher,
+ PrincipalId => $watcher->id,
Silent => 1,
);
push @non_fatal_errors, $self->loc("Couldn't set [_1] watcher: [_2]", $type, $msg)
@@ -616,29 +548,7 @@ sub Create {
# }}}
-
- # Now that we've created the ticket and set up its metadata, we can
- # actually go and check OwnTicket on the ticket itself. This might
- # be different than before in cases where extensions like RTIR are
- # doing clever things with RT's ACL system.
- if ($DeferOwner) {
- if ( $DeferOwner->HasRight( Object => $self, Right => 'OwnTicket' ) ) {
- $self->OwnerGroup->_AddMember(
- PrincipalId => $DeferOwner->PrincipalId,
- InsideTransaction => 1,
- RecordTransaction => 0,
- Object => $self,
- );
- } else {
- $RT::Logger->warning( "User " . $DeferOwner->Name . "(" . $DeferOwner->id
- . ") was proposed as a ticket owner but has no rights to own "
- . "tickets in " . $QueueObj->Name );
- push @non_fatal_errors, $self->loc(
- "Owner '[_1]' does not have rights to own this ticket.",
- $DeferOwner->Name
- );
- }
- }
+ # XXX: Deferred ownership
if ( $args{'_RecordTransaction'} ) {
commit 1b2d3f2799ee96b37f99a6d0ff298fc4f6ce2e21
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 29 04:06:32 2012 -0500
Iteratively add roles, as their addition may impact rights
Replace the "deferred owner" concept with a more generalized iterative
application of roles, as rights permit. Once a pass is completed during
which no roles are added, any remaining un-applied roles that exist are
ones which the current user has no rights to set.
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index d38d8c6..c7e267c 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2350,6 +2350,40 @@ sub _CreateRoleGroups {
return(1);
}
+sub _AddRolesOnCreate {
+ my $self = shift;
+ my ($roles, %acls) = @_;
+
+ my @errors;
+ {
+ my $changed = 0;
+
+ for my $role (keys %{$roles}) {
+ my @left;
+ for my $principal (@{$roles->{$role}}) {
+ if ($acls{$role}->($principal)) {
+ my ($ok, $msg) = $self->RoleGroup($role)->_AddMember(
+ PrincipalId => $principal->id,
+ InsideTransaction => 1,
+ RecordTransaction => 0,
+ Object => $self,
+ );
+ push @errors, $self->loc("Couldn't set [_1] watcher: [_2]", $role, $msg)
+ unless $ok;
+ $changed++;
+ } else {
+ push @left, $principal;
+ }
+ }
+ $roles->{$role} = [ @left ];
+ }
+
+ redo if $changed;
+ }
+
+ return @errors;
+}
+
RT::Base->_ImportOverlays();
1;
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 2c0f2dd..519d7a8 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -356,7 +356,6 @@ sub Create {
# Figure out users for roles
my $roles = {};
push @non_fatal_errors, $self->_ResolveRoles( $roles, %args );
- $args{$_} = $roles->{$_} for keys %{ $roles };
$RT::Handle->BeginTransaction();
@@ -427,39 +426,27 @@ sub Create {
);
}
- # Set the owner group; this also sets the denormalized Owner field
- # appropriately.
- if ($args{Owner}->HasRight( Object => $self, Right => 'OwnTicket' ) ) {
- $self->OwnerGroup->_AddMember(
- PrincipalId => $args{Owner}->Id,
- InsideTransaction => 1,
- RecordTransaction => 0,
- Object => $self,
- );
- }
-
- # Deal with setting up watchers
-
- foreach my $type ( "Cc", "AdminCc", "Requestor" ) {
- # we know it's an array ref
- foreach my $watcher ( @{ $args{$type} } ) {
+ # Codify what it takes to add each kind of group
+ my %acls = (
+ Cc => sub { 1 },
+ Requestor => sub { 1 },
+ AdminCc => sub {
+ my $principal = shift;
+ return 1 if $self->CurrentUserHasRight('ModifyTicket');
+ return $principal->id == $self->CurrentUser->PrincipalId
+ and $self->CurrentUserHasRight("WatchAsAdminCc");
+ },
+ Owner => sub {
+ my $principal = shift;
+ return 1 if $principal->id == RT->Nobody->PrincipalId;
+ return $principal->HasRight( Object => $self, Right => 'OwnTicket' );
+ },
+ );
- # Note that we're using AddWatcher, rather than _AddWatcher, as we
- # actually _want_ that ACL check. Otherwise, random ticket creators
- # could make themselves adminccs and maybe get ticket rights. that would
- # be poor
- my $method = $type eq 'AdminCc'? 'AddWatcher': '_AddWatcher';
-
- my ($val, $msg) = $self->$method(
- Type => $type,
- PrincipalId => $watcher->id,
- Silent => 1,
- );
- push @non_fatal_errors, $self->loc("Couldn't set [_1] watcher: [_2]", $type, $msg)
- unless $val;
- }
- }
+ # Populate up the role groups. This call modifies $roles.
+ push @non_fatal_errors, $self->_AddRolesOnCreate( $roles, %acls );
+ # Squelching
if ($args{'SquelchMailTo'}) {
my @squelch = ref( $args{'SquelchMailTo'} ) ? @{ $args{'SquelchMailTo'} }
: $args{'SquelchMailTo'};
@@ -548,7 +535,27 @@ sub Create {
# }}}
- # XXX: Deferred ownership
+ # Try to add roles once more.
+ push @non_fatal_errors, $self->_AddRolesOnCreate( $roles, %acls );
+
+ # Anything left is failure of ACLs; Cc and Requestor have no ACLs,
+ # so we don't bother checking them.
+ if (@{ $roles->{Owner} }) {
+ my $owner = $roles->{Owner}[0]->Object;
+ $RT::Logger->warning( "User " . $owner->Name . "(" . $owner->id
+ . ") was proposed as a ticket owner but has no rights to own "
+ . "tickets in " . $QueueObj->Name );
+ push @non_fatal_errors, $self->loc(
+ "Owner '[_1]' does not have rights to own this ticket.",
+ $owner->Name
+ );
+ }
+ for my $principal (@{ $roles->{AdminCc} }) {
+ push @non_fatal_errors, $self->loc(
+ "No rights to add '[_1]' as an AdminCc on this ticket",
+ $principal->Object->Name
+ );
+ }
if ( $args{'_RecordTransaction'} ) {
commit 7e0447e31c1cea8e06cbff63566826a14579f24f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 29 04:07:00 2012 -0500
Remove closing fold markers; these have no opening fold
The opening folds were removed in 78554fe, but their corresponding
closing folds remained.
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 519d7a8..ca6745d 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -343,16 +343,11 @@ sub Create {
$Resolved->SetToNow;
}
- # }}}
-
# Dealing with time fields
-
$args{'TimeEstimated'} = 0 unless defined $args{'TimeEstimated'};
$args{'TimeWorked'} = 0 unless defined $args{'TimeWorked'};
$args{'TimeLeft'} = 0 unless defined $args{'TimeLeft'};
- # }}}
-
# Figure out users for roles
my $roles = {};
push @non_fatal_errors, $self->_ResolveRoles( $roles, %args );
@@ -453,11 +448,7 @@ sub Create {
$self->_SquelchMailTo( @squelch );
}
-
- # }}}
-
# Add all the custom fields
-
foreach my $arg ( keys %args ) {
next unless $arg =~ /^CustomField-(\d+)$/i;
my $cfid = $1;
@@ -480,8 +471,6 @@ sub Create {
}
}
- # }}}
-
# Deal with setting up links
# TODO: Adding link may fire scrips on other end and those scrips
@@ -533,8 +522,6 @@ sub Create {
}
}
- # }}}
-
# Try to add roles once more.
push @non_fatal_errors, $self->_AddRolesOnCreate( $roles, %acls );
@@ -590,8 +577,6 @@ sub Create {
}
$RT::Handle->Commit();
return ( $self->Id, $TransObj->Id, $ErrStr );
-
- # }}}
}
else {
@@ -784,8 +769,6 @@ sub Import {
$Owner->Load( RT->Nobody->UserObj->Id );
}
- # }}}
-
unless ( $self->ValidateStatus( $args{'Status'} ) ) {
return ( 0, $self->loc("'[_1]' is an invalid value for status", $args{'Status'}) );
}
@@ -1101,8 +1084,6 @@ sub DeleteWatcher {
}
}
- # }}}
-
# see if this user is already a watcher.
unless ( $group->HasMember($principal) ) {
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list