[Rt-commit] r4158 - in rt/branches/QUEBEC-EXPERIMENTAL: . lib/RT

jesse at bestpractical.com jesse at bestpractical.com
Wed Nov 30 15:57:36 EST 2005


Author: jesse
Date: Wed Nov 30 15:57:35 2005
New Revision: 4158

Modified:
   rt/branches/QUEBEC-EXPERIMENTAL/   (props changed)
   rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Groups_Overlay.pm
   rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Principal_Overlay.pm
   rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Users_Overlay.pm
Log:
 r19556 at truegrounds:  jesse | 2005-11-30 15:28:09 -0500
  r18411 at truegrounds:  jesse | 2005-11-06 17:13:33 -0500
   * Patch to significantly improve performance on "WhoHaveRight" from Ruslan.
 


Modified: rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Groups_Overlay.pm
==============================================================================
--- rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Groups_Overlay.pm	(original)
+++ rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Groups_Overlay.pm	Wed Nov 30 15:57:35 2005
@@ -77,6 +77,13 @@
 use strict;
 no warnings qw(redefine);
 
+use RT::Users;
+
+# XXX: below some code is marked as subject to generalize in Groups, Users classes.
+# RUZ suggest name Principals::Generic or Principals::Base as abstract class, but
+# Jesse wants something that doesn't imply it's a Principals.pm subclass.
+# See comments below for candidats.
+
 
 # {{{ sub _Init
 
@@ -85,15 +92,46 @@
   $self->{'table'} = "Groups";
   $self->{'primary_key'} = "id";
 
+  my @result = $self->SUPER::_Init(@_);
+
   $self->OrderBy( ALIAS => 'main',
 		  FIELD => 'Name',
 		  ORDER => 'ASC');
 
+  $self->{'princalias'} = $self->NewAlias('Principals');
+
+  # XXX: this code should be generalized
+  $self->Join( ALIAS1 => 'main',
+               FIELD1 => 'id',
+               ALIAS2 => $self->{'princalias'},
+               FIELD2 => 'id' );
+
+  # even if this condition is useless and ids in the Groups table
+  # only match principals with type 'Group' this could speed up
+  # searches in some DBs.
+  $self->Limit( ALIAS => $self->{'princalias'},
+                FIELD => 'PrincipalType',
+                VALUE => 'Group',
+              );
 
-  return ( $self->SUPER::_Init(@_));
+  return (@result);
 }
 # }}}
 
+=head2 PrincipalsAlias
+
+Returns the string that represents this Users object's primary "Principals" alias.
+
+=cut
+
+# XXX: should be generalized, code duplication
+sub PrincipalsAlias {
+    my $self = shift;
+    return($self->{'princalias'});
+
+}
+
+
 # {{{ LimiToSystemInternalGroups
 
 =head2 LimitToSystemInternalGroups
@@ -285,7 +323,7 @@
 $groups = RT::Groups->new($RT::SystemUser);
 $groups->WithRight(Right => 'OwnTicket', Object => $q);
 ok ($id,$msg);
-is($groups->Count, 2);
+is($groups->Count, 3);
 
 my $RTxGroup = RT::Group->new($RT::SystemUser);
 ($id, $msg) = $RTxGroup->CreateUserDefinedGroup( Name => 'RTxGroup', Description => "RTx extension group");
@@ -343,83 +381,65 @@
 
 =cut
 
-
+#XXX: should be generilized
 sub WithRight {
     my $self = shift;
     my %args = ( Right                  => undef,
                  Object =>              => undef,
                  IncludeSystemRights    => 1,
                  IncludeSuperusers      => undef,
+                 IncludeSubgroupMembers => 0,
                  EquivObjects           => [ ],
                  @_ );
 
-    my $acl        = $self->NewAlias('ACL');
+    my $from_role = $self->Clone;
+    $from_role->WithRoleRight( %args );
 
-    # {{{ Find only rows where the right granted is the one we're looking up or _possibly_ superuser 
-    $self->Limit( ALIAS           => $acl,
-                  FIELD           => 'RightName',
-                  OPERATOR        => ($args{Right} ? '=' : 'IS NOT'),
-                  VALUE           => $args{Right} || 'NULL',
-                  ENTRYAGGREGATOR => 'OR' );
-
-    if ( $args{'IncludeSuperusers'} and $args{'Right'} ) {
-        $self->Limit( ALIAS           => $acl,
-                      FIELD           => 'RightName',
-                      OPERATOR        => '=',
-                      VALUE           => 'SuperUser',
-                      ENTRYAGGREGATOR => 'OR' );
-    }
-    # }}}
-
-    my ($or_check_ticket_roles, $or_check_roles);
-    my $which_object = "$acl.ObjectType = 'RT::System'";
+    my $from_group = $self->Clone;
+    $from_group->WithGroupRight( %args );
 
-    if ( defined $args{'Object'} ) {
-        if ( ref($args{'Object'}) eq 'RT::Ticket' ) {
-            $or_check_ticket_roles =
-                " OR ( main.Domain = 'RT::Ticket-Role' AND main.Instance = " . $args{'Object'}->Id . ") ";
-
-            # If we're looking at ticket rights, we also want to look at the associated queue rights.
-            # this is a little bit hacky, but basically, now that we've done the ticket roles magic,
-            # we load the queue object and ask all the rest of our questions about the queue.
-            $args{'Object'}   = $args{'Object'}->QueueObj;
-        }
-        # TODO XXX This really wants some refactoring
-        if ( ref($args{'Object'}) eq 'RT::Queue' ) {
-            $or_check_roles =
-                " OR ( ( (main.Domain = 'RT::Queue-Role' AND main.Instance = " .
-                $args{'Object'}->Id . ") $or_check_ticket_roles ) " .
-                " AND main.Type = $acl.PrincipalType) ";
-        }
+    #XXX: DIRTY HACK
+    use DBIx::SearchBuilder::Union;
+    my $union = new DBIx::SearchBuilder::Union;
+    $union->add($from_role);
+    $union->add($from_group);
+    %$self = %$union;
+    bless $self, ref($union);
 
-	if ( $args{'IncludeSystemRights'} ) {
-	    $which_object .= ' OR ';
-	}
-	else {
-	    $which_object = '';
-	}
-        foreach my $obj ( @{ $args{'EquivObjects'} } ) {
-             next unless ( UNIVERSAL::can( $obj, 'id' ) );
-             $which_object .= "($acl.ObjectType = '" . ref( $obj ) . "' AND $acl.ObjectId = " . $obj->id . ") OR ";
-         }
-        $which_object .=
-            " ($acl.ObjectType = '" . ref($args{'Object'}) . "'" .
-            " AND $acl.ObjectId = " . $args{'Object'}->Id . ") ";
-    }
-
-    $self->_AddSubClause( "WhichObject", "($which_object)" );
+    return;
+}
 
-    $self->_AddSubClause( "WhichGroup",
-        qq{
-          ( (    $acl.PrincipalId = main.id
-             AND $acl.PrincipalType = 'Group'
-             AND (   main.Domain = 'SystemInternal'
-                  OR main.Domain = 'UserDefined'
-                  OR main.Domain = 'ACLEquivalence'))
-           $or_check_roles)
-        }
-    );
+#XXX: methods are active aliases to Users class to prevent code duplication
+# should be generalized
+sub _JoinGroups {
+    my $self = shift;
+    my %args = (@_);
+    return 'main' unless $args{'IncludeSubgroupMembers'};
+    return $self->RT::Users::_JoinGroups( %args );
+}
+sub _JoinGroupMembers {
+    my $self = shift;
+    my %args = (@_);
+    return 'main' unless $args{'IncludeSubgroupMembers'};
+    return $self->RT::Users::_JoinGroupMembers( %args );
 }
+sub _JoinGroupMembersForGroupRights {
+    my $self = shift;
+    my %args = (@_);
+    my $group_members = $self->_JoinGroupMembers( %args );
+    unless( $group_members eq 'main' ) {
+        return $self->RT::Users::_JoinGroupMembersForGroupRights( %args );
+    }
+    $self->Limit( ALIAS => $args{'ACLAlias'},
+                  FIELD => 'PrincipalId',
+                  VALUE => "main.id",
+                  QUOTEVALUE => 0,
+                );
+}
+sub _JoinACL          { return (shift)->RT::Users::_JoinACL( @_ ) }
+sub _GetEquivObjects  { return (shift)->RT::Users::_GetEquivObjects( @_ ) }
+sub WithGroupRight    { return (shift)->RT::Users::WhoHaveGroupRight( @_ ) }
+sub WithRoleRight     { return (shift)->RT::Users::WhoHaveRoleRight( @_ ) }
 
 # {{{ sub LimitToEnabled
 
@@ -432,18 +452,11 @@
 sub LimitToEnabled {
     my $self = shift;
     
-    my $alias = $self->Join(
-	TYPE   => 'left',
-	ALIAS1 => 'main',
-	FIELD1 => 'id',
-	TABLE2 => 'Principals',
-	FIELD2 => 'id'
-    );
-
-    $self->Limit( ALIAS => $alias,
-		  FIELD => 'Disabled',
-		  VALUE => '0',
-		  OPERATOR => '=' );
+    $self->Limit( ALIAS => $self->PrincipalsAlias,
+		          FIELD => 'Disabled',
+		          VALUE => '0',
+		          OPERATOR => '=',
+                );
 }
 # }}}
 
@@ -458,20 +471,12 @@
 sub LimitToDeleted {
     my $self = shift;
     
-    my $alias = $self->Join(
-	TYPE   => 'left',
-	ALIAS1 => 'main',
-	FIELD1 => 'id',
-	TABLE2 => 'Principals',
-	FIELD2 => 'id'
-    );
-
     $self->{'find_disabled_rows'} = 1;
-    $self->Limit( ALIAS => $alias,
-		  FIELD => 'Disabled',
-		  OPERATOR => '=',
-		  VALUE => '1'
-		);
+    $self->Limit( ALIAS => $self->PrincipalsAlias,
+                  FIELD => 'Disabled',
+                  OPERATOR => '=',
+                  VALUE => 1,
+                );
 }
 # }}}
 

Modified: rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Principal_Overlay.pm
==============================================================================
--- rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Principal_Overlay.pm	(original)
+++ rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Principal_Overlay.pm	Wed Nov 30 15:57:35 2005
@@ -280,8 +280,6 @@
     Object => an RT style object (->id will get its id)
 
 
-
-
 Returns 1 if a matching ACE was found.
 
 Returns undef if no ACE was found.
@@ -295,38 +293,39 @@
         Right        => undef,
         Object       => undef,
         EquivObjects => undef,
-        @_
+        @_,
     );
 
+    unless ( $args{'Right'} ) {
+        $RT::Logger->crit("HasRight called without a right");
+        return (undef);
+    }
+
     if ( $self->Disabled ) {
-        $RT::Logger->err( "Disabled User:  "
+        $RT::Logger->error( "Disabled User:  "
               . $self->id
               . " failed access check for "
               . $args{'Right'} );
         return (undef);
     }
 
-    if ( !defined $args{'Right'} ) {
-        $RT::Logger->crit("HasRight called without a right");
-        return (undef);
-    }
-
     if (   defined( $args{'Object'} )
         && UNIVERSAL::can( $args{'Object'}, 'id' )
-        && $args{'Object'}->id )
-    {
+        && $args{'Object'}->id ) {
+
         push( @{ $args{'EquivObjects'} }, $args{Object} );
     }
     else {
-        $RT::Logger->crit("$self HasRight called with no valid object");
+        $RT::Logger->crit("HasRight called with no valid object");
         return (undef);
     }
 
     # If this object is a ticket, we care about ticket roles and queue roles
-    if ( ( ref( $args{'Object'} ) eq 'RT::Ticket' ) && $args{'Object'}->Id ) {
+    if ( UNIVERSAL::isa( $args{'Object'} => 'RT::Ticket' ) ) {
 
-# this is a little bit hacky, but basically, now that we've done the ticket roles magic, we load the queue object
-# and ask all the rest of our questions about the queue.
+        # this is a little bit hacky, but basically, now that we've done
+        # the ticket roles magic, we load the queue object
+        # and ask all the rest of our questions about the queue.
         push( @{ $args{'EquivObjects'} }, $args{'Object'}->QueueObj );
 
     }
@@ -354,161 +353,119 @@
 
     # }}}
 
-    # {{{ if we've cached a positive result for this query, return 1
-
-    my $cached_answer = $_ACL_CACHE->fetch($hashkey);
-
     # Returns undef on cache miss
+    my $cached_answer = $_ACL_CACHE->fetch($hashkey);
     if ( defined $cached_answer ) {
         if ( $cached_answer == 1 ) {
             return (1);
         }
         elsif ( $cached_answer == -1 ) {
-            return (0);
+            return (undef);
         }
     }
 
-    my ( $or_look_at_object_rights, $or_check_roles );
-    my $right = $args{'Right'};
+    my $hitcount = $self->_HasRight( %args );
 
-    # {{{ Construct Right Match
+    $_ACL_CACHE->set( $hashkey => $hitcount? 1:-1 );
+    return ($hitcount);
+}
+
+=head2 _HasRight
+
+Low level HasRight implementation, use HasRight method instead.
+
+=cut
+
+sub _HasRight
+{
+    my $self = shift;
+    my %args = (
+        Right        => undef,
+        Object       => undef,
+        EquivObjects => [],
+        @_
+    );
+
+    my $right = $args{'Right'};
+    my @objects = @{ $args{'EquivObjects'} };
 
     # If an object is defined, we want to look at rights for that object
 
-    my @look_at_objects;
-    push( @look_at_objects, "ACL.ObjectType = 'RT::System'" )
+    push( @objects, 'RT::System' )
       unless $self->can('_IsOverrideGlobalACL')
-      and $self->_IsOverrideGlobalACL( $args{Object} );
+             && $self->_IsOverrideGlobalACL( $args{Object} );
 
-    foreach my $obj ( @{ $args{'EquivObjects'} } ) {
-        next unless ( UNIVERSAL::can( $obj, 'id' ) );
-        my $type = ref($obj);
-        my $id   = $obj->id;
-
-        unless ($id) {
-            use Carp;
-            Carp::cluck(
-                "Trying to check $type rights for an unspecified $type");
-            $RT::Logger->crit(
-                "Trying to check $type rights for an unspecified $type");
+    my ($check_roles, $check_objects) = ('','');
+    if( @objects ) {
+        my @role_clauses;
+        my @object_clauses;
+        foreach my $obj ( @objects ) {
+            my $type = ref($obj)? ref($obj): $obj;
+            my $id;
+            $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;
+            push @role_clauses, "($role_clause)";
+
+            my $object_clause = "ACL.ObjectType = '$type'";
+            $object_clause   .= " AND ACL.ObjectId = $id" if $id;
+            push @object_clauses, "($object_clause)";
         }
-        push @look_at_objects,
-          "(ACL.ObjectType = '$type' AND ACL.ObjectId = '$id')";
-    }
-
-    # }}}
 
-    # {{{ Build that honkin-big SQL query
+        $check_roles .= join ' OR ', @role_clauses;
+        $check_objects = join ' OR ', @object_clauses;
+    }
 
     my $query_base =
       "SELECT ACL.id from ACL, Groups, Principals, CachedGroupMembers WHERE  " .
 
       # Only find superuser or rights with the name $right
-      "(ACL.RightName = 'SuperUser' OR  ACL.RightName = '$right') " .
+      "(ACL.RightName = 'SuperUser' OR  ACL.RightName = '$right') "
 
       # Never find disabled groups.
-      "AND Principals.Disabled = 0 "
-      . "AND CachedGroupMembers.Disabled = 0  "
-      . "AND Principals.id = Groups.id "
-      .    # We always grant rights to Groups
-
-# See if the principal is a member of the group recursively or _is the rightholder_
-# never find recursively disabled group members
-# also, check to see if the right is being granted _directly_ to this principal,
-#  as is the case when we want to look up group rights
-"AND  Principals.id = CachedGroupMembers.GroupId AND CachedGroupMembers.MemberId = '"
-      . $self->Id . "' "
-      .
-
-  # Make sure the rights apply to the entire system or to the object in question
-      "AND ( " . join( ' OR ', @look_at_objects ) . ") ";
-
-# The groups query does the query based on group membership and individual user rights
-
-    my $groups_query = $query_base .
-
-# limit the result set to groups of types ACLEquivalence (user)  UserDefined, SystemInternal and Personal
-"AND ( (  ACL.PrincipalId = Principals.id AND ACL.PrincipalType = 'Group' AND "
-      . "(Groups.Domain = 'SystemInternal' OR Groups.Domain = 'UserDefined' OR Groups.Domain = 'ACLEquivalence' OR Groups.Domain = 'Personal'))"
-      .
-
-      " ) ";
-    $self->_Handle->ApplyLimits( \$groups_query, 1 );    #only return one result
-
-    my @roles;
-    foreach my $object ( @{ $args{'EquivObjects'} } ) {
-        push( @roles, $self->_RolesForObject( ref($object), $object->id ) );
-    }
+      . "AND Principals.Disabled = 0 "
+      . "AND CachedGroupMembers.Disabled = 0 "
 
-    # The roles query does the query based on roles
-    my $roles_query;
-    if (@roles) {
-        $roles_query =
-            $query_base . "AND " . " ( ("
-          . join( ' OR ', @roles ) . " ) "
-          . " AND Groups.Type = ACL.PrincipalType AND Groups.Id = Principals.id AND Principals.PrincipalType = 'Group') ";
-        $self->_Handle->ApplyLimits( \$roles_query, 1 ); #only return one result
+      # We always grant rights to Groups
+      . "AND Principals.id = Groups.id "
+      . "AND Principals.PrincipalType = 'Group' "
 
-    }
+      # See if the principal is a member of the group recursively or _is the rightholder_
+      # never find recursively disabled group members
+      # also, check to see if the right is being granted _directly_ to this principal,
+      #  as is the case when we want to look up group rights
+      . "AND Principals.id = CachedGroupMembers.GroupId "
+      . "AND CachedGroupMembers.MemberId = ". $self->Id ." "
+
+      # Make sure the rights apply to the entire system or to the object in question
+      . "AND ($check_objects) ";
+
+    # The groups query does the query based on group membership and individual user rights
+    my $groups_query = $query_base
+      # limit the result set to groups of types ACLEquivalence (user),
+      # UserDefined, SystemInternal and Personal. All this we do
+      # via (ACL.PrincipalType = 'Group') condition
+      . "AND ACL.PrincipalId = Principals.id "
+      . "AND ACL.PrincipalType = 'Group' ";
 
-    # }}}
-
-    # {{{ Actually check the ACL by performing an SQL query
-    #   $RT::Logger->debug("Now Trying $groups_query");
+    $self->_Handle->ApplyLimits( \$groups_query, 1 ); #only return one result
     my $hitcount = $self->_Handle->FetchResult($groups_query);
+    return 1 if $hitcount; # get out of here if success
 
-    # }}}
-
-    # {{{ if there's a match, the right is granted
-    if ($hitcount) {
-        $_ACL_CACHE->set( $hashkey => 1 );
-        return (1);
-    }
+    # The roles query does the query based on roles
+    my $roles_query = $query_base
+      . "AND ACL.PrincipalType = Groups.Type "
+      . "AND ($check_roles) ";
+    $self->_Handle->ApplyLimits( \$roles_query, 1 ); #only return one result
 
-    # Now check the roles query
     $hitcount = $self->_Handle->FetchResult($roles_query);
+    return 1 if $hitcount; # get out of here if success
 
-    if ($hitcount) {
-        $_ACL_CACHE->set( $hashkey => 1 );
-        return (1);
-    }
-
-    # We failed to find an acl hit
-    $_ACL_CACHE->set( $hashkey => -1 );
-    return (undef);
-}
-
-# }}}
-
-# {{{ _RolesForObject
-
-
-
-=head2 _RolesForObject( $object_type, $object_id)
-
-Returns an SQL clause finding role groups for Objects
-
-=cut
-
-
-sub _RolesForObject {
-    my $self = shift;
-    my $type = shift;
-    my $id = shift;
-
-    unless ($id) {
-	$id = '0';
-   }
-
-   # This should never be true.
-   unless ($id =~ /^\d+$/) {
-	$RT::Logger->crit("RT::Prinicipal::_RolesForObject called with type $type and a non-integer id: '$id'");
-	$id = "'$id'";
-   }
-
-    my $clause = "(Groups.Domain = '".$type."-Role' AND Groups.Instance = $id) ";
-
-    return($clause);
+    return 0;
 }
 
 # }}}
@@ -578,6 +535,8 @@
     # just return the value for non-objects
     return $scalar unless UNIVERSAL::can($scalar, 'id');
 
+    return ref($scalar) unless $scalar->id;
+
     # an object -- return the class and id
     return(ref($scalar)."-". $scalar->id);
 }

Modified: rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Users_Overlay.pm
==============================================================================
--- rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Users_Overlay.pm	(original)
+++ rt/branches/QUEBEC-EXPERIMENTAL/lib/RT/Users_Overlay.pm	Wed Nov 30 15:57:35 2005
@@ -88,10 +88,15 @@
 
     $self->{'princalias'} = $self->NewAlias('Principals');
 
+    # XXX: should be generalized
     $self->Join( ALIAS1 => 'main',
                  FIELD1 => 'id',
                  ALIAS2 => $self->{'princalias'},
                  FIELD2 => 'id' );
+    $self->Limit( ALIAS => $self->{'princalias'},
+                  FIELD => 'PrincipalType',
+                  VALUE => 'User',
+                );
 
     return (@result);
 }
@@ -102,9 +107,9 @@
 
 Returns the string that represents this Users object's primary "Principals" alias.
 
-
 =cut
 
+# XXX: should be generalized
 sub PrincipalsAlias {
     my $self = shift;
     return($self->{'princalias'});
@@ -141,10 +146,11 @@
 
 =cut
 
+# XXX: should be generalized
 sub LimitToEnabled {
     my $self = shift;
 
-    $self->Limit( ALIAS    => $self->{'princalias'},
+    $self->Limit( ALIAS    => $self->PrincipalsAlias,
                   FIELD    => 'Disabled',
                   VALUE    => '0',
                   OPERATOR => '=' );
@@ -187,7 +193,7 @@
     my $groupalias = $self->NewAlias('CachedGroupMembers');
 
     # Join the principal to the groups table
-    $self->Join( ALIAS1 => $self->{'princalias'},
+    $self->Join( ALIAS1 => $self->PrincipalsAlias,
                  FIELD1 => 'id',
                  ALIAS2 => $groupalias,
                  FIELD2 => 'MemberId' );
@@ -288,69 +294,75 @@
 
 =end testing
 
-
 find all users who the right Right for this group, either individually
 or as members of groups
 
-
 If passed a queue object, with no id, it will find users who have that right for _any_ queue
 
-
-
 =cut
 
-sub WhoHaveRight {
+# XXX: should be generalized
+sub _JoinGroupMembers
+{
     my $self = shift;
     my %args = (
-        Right                  => undef,
-        Object                 => undef,
-        IncludeSystemRights    => undef,
-        IncludeSuperusers      => undef,
         IncludeSubgroupMembers => 1,
-        EquivObjects           => [ ],
         @_
     );
 
-    if ( defined $args{'ObjectType'} || defined $args{'ObjectId'} ) {
-        $RT::Logger->crit( "$self WhoHaveRight called with the Obsolete ObjectId/ObjectType API");
-        return (undef);
-    }
-    
-
-    # Find only members of groups that have the right.
-
-    my $acl       = $self->NewAlias('ACL');
-    my $groups    = $self->NewAlias('Groups');
-    my $userprinc = $self->{'princalias'};
-
-# The cachedgroupmembers table is used for unrolling group memberships to allow fast lookups
-# if we bind to CachedGroupMembers, we'll find all members of groups recursively.
-# if we don't we'll find only 'direct' members of the group in question
-    my $cgm;
+    my $principals = $self->PrincipalsAlias;
 
+    # The cachedgroupmembers table is used for unrolling group memberships
+    # to allow fast lookups. if we bind to CachedGroupMembers, we'll find
+    # all members of groups recursively. if we don't we'll find only 'direct'
+    # members of the group in question
+    my $group_members;
     if ( $args{'IncludeSubgroupMembers'} ) {
-        $cgm = $self->NewAlias('CachedGroupMembers');
+        $group_members = $self->NewAlias('CachedGroupMembers');
     }
     else {
-        $cgm = $self->NewAlias('GroupMembers');
+        $group_members = $self->NewAlias('GroupMembers');
     }
 
-#Tie the users we're returning ($userprinc) to the groups that have rights granted to them ($groupprinc)
     $self->Join(
-        ALIAS1 => $cgm,
+        ALIAS1 => $group_members,
         FIELD1 => 'MemberId',
-        ALIAS2 => $userprinc,
+        ALIAS2 => $principals,
         FIELD2 => 'id'
     );
 
+    return $group_members;
+}
+
+# XXX: should be generalized
+sub _JoinGroups
+{
+    my $self = shift;
+    my %args = (@_);
+
+    my $group_members = $self->_JoinGroupMembers( %args );
+    my $groups = $self->NewAlias('Groups');
     $self->Join(
         ALIAS1 => $groups,
         FIELD1 => 'id',
-        ALIAS2 => $cgm,
+        ALIAS2 => $group_members,
         FIELD2 => 'GroupId'
     );
 
-# {{{ Find only rows where the right granted is the one we're looking up or _possibly_ superuser
+    return $groups;
+}
+
+# XXX: should be generalized
+sub _JoinACL
+{
+    my $self = shift;
+    my %args = (
+        Right                  => undef,
+        IncludeSuperusers      => undef,
+        @_,
+    );
+
+    my $acl = $self->NewAlias('ACL');
     $self->Limit(
         ALIAS    => $acl,
         FIELD    => 'RightName',
@@ -358,7 +370,6 @@
         VALUE => $args{Right} || 'NULL',
         ENTRYAGGREGATOR => 'OR'
     );
-
     if ( $args{'IncludeSuperusers'} and $args{'Right'} ) {
         $self->Limit(
             ALIAS           => $acl,
@@ -368,97 +379,240 @@
             ENTRYAGGREGATOR => 'OR'
         );
     }
+    return $acl;
+}
 
-    # }}}
-
-    my ( $or_check_ticket_roles, $or_check_roles );
-    my $which_object = "$acl.ObjectType = 'RT::System'";
+# XXX: should be generalized
+sub _GetEquivObjects
+{
+    my $self = shift;
+    my %args = (
+        Object                 => undef,
+        IncludeSystemRights    => undef,
+        EquivObjects           => [ ],
+        @_
+    );
+    return () unless $args{'Object'};
 
-    if ( defined $args{'Object'} ) {
-        if ( ref( $args{'Object'} ) eq 'RT::Ticket' ) {
-            $or_check_ticket_roles = " OR ( $groups.Domain = 'RT::Ticket-Role' AND $groups.Instance = " . $args{'Object'}->Id . ") ";
-
-# If we're looking at ticket rights, we also want to look at the associated queue rights.
-# this is a little bit hacky, but basically, now that we've done the ticket roles magic,
-# we load the queue object and ask all the rest of our questions about the queue.
-            $args{'Object'} = $args{'Object'}->QueueObj;
+    my @objects = ($args{'Object'});
+    if ( UNIVERSAL::isa( $args{'Object'}, 'RT::Ticket' ) ) {
+        # If we're looking at ticket rights, we also want to look at the associated queue rights.
+        # this is a little bit hacky, but basically, now that we've done the ticket roles magic,
+        # we load the queue object and ask all the rest of our questions about the queue.
+
+        # XXX: This should be abstracted into object itself
+        if( $args{'Object'}->id ) {
+            push @objects, $args{'Object'}->QueueObj;
+        } else {
+            push @objects, 'RT::Queue';
         }
+    }
 
-        # TODO XXX This really wants some refactoring
-        if ( ref( $args{'Object'} ) eq 'RT::Queue' ) {
-            $or_check_roles = " OR ( ( ($groups.Domain = 'RT::Queue-Role' ";
-            $or_check_roles .= "AND $groups.Instance = " . $args{'Object'}->id if ( $args{'Object'}->id );
-            $or_check_roles .= ") $or_check_ticket_roles ) " . " AND $groups.Type = $acl.PrincipalType) ";
-        }
-        if ( $args{'IncludeSystemRights'} ) {
-            $which_object .= ' OR ';
-        }
-        else {
-            $which_object = '';
-        }
-        foreach my $obj ( @{ $args{'EquivObjects'} } ) {
-            $which_object .= "($acl.ObjectType = '" . ref( $obj ) . "' AND $acl.ObjectId = " . $obj->id . ") OR ";
-        }
-        $which_object .= " ($acl.ObjectType = '" . ref( $args{'Object'} ) . "'";
-        if ( $args{'Object'}->id ) {
-            $which_object .= " AND $acl.ObjectId = " . $args{'Object'}->id;
-        }
+    if( $args{'IncludeSystemRights'} ) {
+        push @objects, 'RT::System';
+    }
+    push @objects, @{ $args{'EquivObjects'} };
+    return grep $_, @objects;
+}
+
+# XXX: should be generalized
+sub WhoHaveRight {
+    my $self = shift;
+    my %args = (
+        Right                  => undef,
+        Object                 => undef,
+        IncludeSystemRights    => undef,
+        IncludeSuperusers      => undef,
+        IncludeSubgroupMembers => 1,
+        EquivObjects           => [ ],
+        @_
+    );
 
-        $which_object .=  ") ";
+    if ( defined $args{'ObjectType'} || defined $args{'ObjectId'} ) {
+        $RT::Logger->crit( "WhoHaveRight called with the Obsolete ObjectId/ObjectType API");
+        return (undef);
     }
-    $self->_AddSubClause( "WhichObject", "($which_object)" );
-    $self->_AddSubClause(
-        "WhichGroup",
-            qq{ ( (    $acl.PrincipalId = $groups.id AND $acl.PrincipalType = 'Group' 
-                AND (   $groups.Domain = 'SystemInternal' OR $groups.Domain = 'UserDefined' OR $groups.Domain = 'ACLEquivalence')) 
-                $or_check_roles) }
+
+    my $from_role = $self->Clone;
+    $from_role->WhoHaveRoleRight( %args );
+
+    my $from_group = $self->Clone;
+    $from_group->WhoHaveGroupRight( %args );
+
+    #XXX: DIRTY HACK
+    use DBIx::SearchBuilder::Union;
+    my $union = new DBIx::SearchBuilder::Union;
+    $union->add($from_role);
+    $union->add($from_group);
+    %$self = %$union;
+    bless $self, ref($union);
+
+    return;
+}
+# }}}
+
+# XXX: should be generalized
+sub WhoHaveRoleRight
+{
+    my $self = shift;
+    my %args = (
+        Right                  => undef,
+        Object                 => undef,
+        IncludeSystemRights    => undef,
+        IncludeSuperusers      => undef,
+        IncludeSubgroupMembers => 1,
+        EquivObjects           => [ ],
+        @_
     );
-    # only include regular RT users
-    $self->LimitToEnabled;
+
+    my $groups = $self->_JoinGroups( %args );
+    my $acl = $self->_JoinACL( %args );
+
+    my ($check_roles, $check_objects) = ('','');
+    
+    my @objects = $self->_GetEquivObjects( %args );
+    if ( @objects ) {
+        my @role_clauses;
+        my @object_clauses;
+        foreach my $obj ( @objects ) {
+            my $type = ref($obj)? ref($obj): $obj;
+            my $id;
+            $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;
+            push @role_clauses, "($role_clause)";
+
+            my $object_clause = "$acl.ObjectType = '$type'";
+            $object_clause   .= " AND $acl.ObjectId = $id" if $id;
+            push @object_clauses, "($object_clause)";
+        }
+
+        $check_roles .= join ' OR ', @role_clauses;
+        $check_objects = join ' OR ', @object_clauses;
+    } else {
+        if( !$args{'IncludeSystemRights'} ) {
+            $check_objects = "($acl.ObjectType != 'RT::System')";
+        }
+    }
+
+    $self->_AddSubClause( "WhichObject", "($check_objects)" );
+    $self->_AddSubClause( "WhichRole", "($check_roles)" );
+
+    $self->Limit( ALIAS => $acl,
+                  FIELD => 'PrincipalType',
+                  VALUE => "$groups.Type",
+                  QUOTEVALUE => 0,
+                );
 
     # no system user
-    $self->Limit( ALIAS => $userprinc, FIELD => 'id', OPERATOR => '!=', VALUE => $RT::SystemUser->id);
+    $self->Limit( ALIAS => $self->PrincipalsAlias,
+                  FIELD => 'id',
+                  OPERATOR => '!=',
+                  VALUE => $RT::SystemUser->id
+                );
+    return;
+}
 
+# XXX: should be generalized
+sub _JoinGroupMembersForGroupRights
+{
+    my $self = shift;
+    my %args = (@_);
+    my $group_members = $self->_JoinGroupMembers( %args );
+    $self->Limit( ALIAS => $args{'ACLAlias'},
+                  FIELD => 'PrincipalId',
+                  VALUE => "$group_members.GroupId",
+                  QUOTEVALUE => 0,
+                );
 }
-# }}}
 
-# {{{ WhoBelongToGroups 
+# XXX: should be generalized
+sub WhoHaveGroupRight
+{
+    my $self = shift;
+    my %args = (
+        Right                  => undef,
+        Object                 => undef,
+        IncludeSystemRights    => undef,
+        IncludeSuperusers      => undef,
+        IncludeSubgroupMembers => 1,
+        EquivObjects           => [ ],
+        @_
+    );
+
+    # Find only rows where the right granted is
+    # the one we're looking up or _possibly_ superuser
+    my $acl = $self->_JoinACL( %args );
+
+    my ($check_objects) = ('');
+    my @objects = $self->_GetEquivObjects( %args );
+
+    if ( @objects ) {
+        my @object_clauses;
+        foreach my $obj ( @objects ) {
+            my $type = ref($obj)? ref($obj): $obj;
+            my $id;
+            $id = $obj->id if ref($obj) && UNIVERSAL::can($obj, 'id') && $obj->id;
+
+            my $object_clause = "$acl.ObjectType = '$type'";
+            $object_clause   .= " AND $acl.ObjectId   = $id" if $id;
+            push @object_clauses, "($object_clause)";
+        }
+
+        $check_objects = join ' OR ', @object_clauses;
+    } else {
+        if( !$args{'IncludeSystemRights'} ) {
+            $check_objects = "($acl.ObjectType != 'RT::System')";
+        }
+    }
+    $self->_AddSubClause( "WhichObject", "($check_objects)" );
+    
+    $self->_JoinGroupMembersForGroupRights( %args, ACLAlias => $acl );
+    # Find only members of groups that have the right.
+    $self->Limit( ALIAS => $acl,
+                  FIELD => 'PrincipalType',
+                  VALUE => 'Group',
+                );
+    
+    # no system user
+    $self->Limit( ALIAS => $self->PrincipalsAlias,
+                  FIELD => 'id',
+                  OPERATOR => '!=',
+                  VALUE => $RT::SystemUser->id
+                );
+    return;
+}
+
+# {{{ WhoBelongToGroups
 
 =head2 WhoBelongToGroups { Groups => ARRAYREF, IncludeSubgroupMembers => 1 }
 
 =cut
 
+# XXX: should be generalized
 sub WhoBelongToGroups {
     my $self = shift;
     my %args = ( Groups                 => undef,
                  IncludeSubgroupMembers => 1,
                  @_ );
 
-    # Unprivileged users can't be granted real system rights. 
+    # Unprivileged users can't be granted real system rights.
     # is this really the right thing to be saying?
     $self->LimitToPrivileged();
 
-    my $userprinc  = $self->{'princalias'};
-    my $cgm;
-
-    # The cachedgroupmembers table is used for unrolling group memberships to allow fast lookups 
-    # if we bind to CachedGroupMembers, we'll find all members of groups recursively.
-    # if we don't we'll find only 'direct' members of the group in question
-
-    if ( $args{'IncludeSubgroupMembers'} ) {
-        $cgm = $self->NewAlias('CachedGroupMembers');
-    }
-    else {
-        $cgm = $self->NewAlias('GroupMembers');
-    }
-
-    #Tie the users we're returning ($userprinc) to the groups that have rights granted to them ($groupprinc)
-    $self->Join( ALIAS1 => $cgm, FIELD1 => 'MemberId',
-                 ALIAS2 => $userprinc, FIELD2 => 'id' );
+    my $group_members = $self->_JoinGroupMembers( %args );
 
     foreach my $groupid (@{$args{'Groups'}}) {
-        $self->Limit(ALIAS => $cgm, FIELD => 'GroupId', VALUE => $groupid, QUOTEVALUE => 0, ENTRYAGGREGATOR=> 'OR')
-
+        $self->Limit( ALIAS           => $group_members,
+                      FIELD           => 'GroupId',
+                      VALUE           => $groupid,
+                      QUOTEVALUE      => 0,
+                      ENTRYAGGREGATOR => 'OR',
+                    );
     }
 }
 # }}}


More information about the Rt-commit mailing list