[Rt-commit] rt branch, 4.0/group-dashboards, updated. rt-4.0.1-128-g0707c18
Alex Vandiver
alexmv at bestpractical.com
Thu Jul 14 19:54:45 EDT 2011
The branch, 4.0/group-dashboards has been updated
via 0707c1866ed8b1556ab29a5abd9b2668302d0788 (commit)
via 9b4f9541312680e57355060e56198f7db16fd27d (commit)
via 86a78900a79961dc4bc0e67a30f70e2016356599 (commit)
from a3438d7e00940b0e05839b8e5ec8e74a5e994b2a (commit)
Summary of changes:
lib/RT/Groups.pm | 28 ++++++---
t/api/group-rights.t | 144 +++++++++++++++++++++++----------------------
t/api/groups.t | 2 +-
t/web/dashboards-groups.t | 14 +---
4 files changed, 96 insertions(+), 92 deletions(-)
- Log -----------------------------------------------------------------
commit 86a78900a79961dc4bc0e67a30f70e2016356599
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Jul 14 15:44:07 2011 -0400
Fix the join direction on ForWhichCurrentUserHasRight
Previously, you only saw the group if your _direct_ membership in it was
what granted the right. However, what we actually care about is if:
(a) You are a member of the group, by any means.
AND
(b) The group grants you the right; that is, if it is a member of any
group which has that right granted to it.
diff --git a/lib/RT/Groups.pm b/lib/RT/Groups.pm
index ddd427b..5a74360 100644
--- a/lib/RT/Groups.pm
+++ b/lib/RT/Groups.pm
@@ -348,13 +348,22 @@ sub ForWhichCurrentUserHasRight {
@_,
);
- my $member = $self->WithMember(
+ # Groups which the current user is a member of
+ $self->WithMember(
PrincipalId => $self->CurrentUser->Id,
Recursively => 1,
);
- my $acl = $self->_JoinACL( %args );
+ # Which are themselves a member of a any group...
+ my $member = $self->Join(
+ ALIAS1 => 'main',
+ FIELD1 => 'id',
+ TABLE2 => 'CachedGroupMembers',
+ FIELD2 => 'MemberId'
+ );
+ # ...with the right in question
+ my $acl = $self->_JoinACL( %args );
$self->Join(
ALIAS1 => $member,
FIELD1 => 'GroupId',
diff --git a/t/api/group-rights.t b/t/api/group-rights.t
index 1219677..172ff0f 100644
--- a/t/api/group-rights.t
+++ b/t/api/group-rights.t
@@ -119,10 +119,7 @@ ok($ok, $msg);
my %has_right = map { ($_->Name => 1) } @{ $groups->ItemsArrayRef };
ok(delete $has_right{Employees}, "Has the right on a group it's in");
- TODO: {
- local $TODO = "Doesn't recurse properly";
- ok(delete $has_right{Hackers}, "Grants the right recursively");
- }
+ ok(delete $has_right{Hackers}, "Grants the right recursively");
ok(not(delete $has_right{Other}), "Doesn't have the right on a group it's in");
ok(not(keys %has_right), "Has no other groups")
}
commit 9b4f9541312680e57355060e56198f7db16fd27d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Jul 14 19:38:51 2011 -0400
Rewrite ForWhichCurrentUserHasRight, and add more tests
The previous implementation didn't depend in any way on what object the
right was granted on, merely the membership of the group, and what
groups that group was in. The correct implementation examines the
ObjectType and ObjectId parameters of the ACL table. Specifically, it
returns groups which are the target of ACLs, which were granted by a
group, which the current user is a member of. That is, groups which the
current user has a particular right on, as the name of the method
implies.
t/api/group-rights.t has been refactored to be more concise, and cover
all possibilities of where the ACL can be granted, and to what.
ForWhichCurrentUserHasRight now agrees with CurrentUserHasRight for all
cases.
diff --git a/lib/RT/Groups.pm b/lib/RT/Groups.pm
index 5a74360..b5df742 100644
--- a/lib/RT/Groups.pm
+++ b/lib/RT/Groups.pm
@@ -348,27 +348,26 @@ sub ForWhichCurrentUserHasRight {
@_,
);
- # Groups which the current user is a member of
- $self->WithMember(
- PrincipalId => $self->CurrentUser->Id,
- Recursively => 1,
- );
+ # Groups which are the target object of an ACL with that right, or
+ # where the target is the system object (a global right)
+ my $acl = $self->_JoinACL( %args );
+ $self->_AddSubClause(
+ ACLObjects => "( (main.id = $acl.ObjectId AND $acl.ObjectType = 'RT::Group')"
+ . " OR $acl.ObjectType = 'RT::System')");
- # Which are themselves a member of a any group...
+ # ...and where that right is granted to any group..
my $member = $self->Join(
- ALIAS1 => 'main',
- FIELD1 => 'id',
+ ALIAS1 => $acl,
+ FIELD1 => 'PrincipalId',
TABLE2 => 'CachedGroupMembers',
- FIELD2 => 'MemberId'
+ FIELD2 => 'GroupId',
);
- # ...with the right in question
- my $acl = $self->_JoinACL( %args );
- $self->Join(
- ALIAS1 => $member,
- FIELD1 => 'GroupId',
- ALIAS2 => $acl,
- FIELD2 => 'PrincipalId',
+ # ...with the current user in it
+ $self->Limit(
+ ALIAS => $member,
+ FIELD => 'MemberId',
+ VALUE => $self->CurrentUser->Id,
);
return;
diff --git a/t/api/group-rights.t b/t/api/group-rights.t
index 172ff0f..ce5aad7 100644
--- a/t/api/group-rights.t
+++ b/t/api/group-rights.t
@@ -6,7 +6,7 @@ RT::Group->AddRights(
'RTxGroupRight' => 'Just a right for testing rights',
);
-# this company is split into two halves, the hacks and the non-hacks
+# this company is split into two halves, the hackers and the non-hackers
# herbert is a hacker but eric is not.
my $herbert = RT::User->new(RT->SystemUser);
my ($ok, $msg) = $herbert->Create(Name => 'herbert');
@@ -16,29 +16,29 @@ my $eric = RT::User->new(RT->SystemUser);
($ok, $msg) = $eric->Create(Name => 'eric');
ok($ok, $msg);
-my $hacks = RT::Group->new(RT->SystemUser);
-($ok, $msg) = $hacks->CreateUserDefinedGroup(Name => 'Hackers');
+my $hackers = RT::Group->new(RT->SystemUser);
+($ok, $msg) = $hackers->CreateUserDefinedGroup(Name => 'Hackers');
ok($ok, $msg);
my $employees = RT::Group->new(RT->SystemUser);
($ok, $msg) = $employees->CreateUserDefinedGroup(Name => 'Employees');
ok($ok, $msg);
-($ok, $msg) = $employees->AddMember($hacks->PrincipalId);
+($ok, $msg) = $employees->AddMember($hackers->PrincipalId);
ok($ok, $msg);
-($ok, $msg) = $hacks->AddMember($herbert->PrincipalId);
+($ok, $msg) = $hackers->AddMember($herbert->PrincipalId);
ok($ok, $msg);
($ok, $msg) = $employees->AddMember($eric->PrincipalId);
ok($ok, $msg);
-ok($employees->HasMemberRecursively($hacks->PrincipalId), 'employees has member hacks');
+ok($employees->HasMemberRecursively($hackers->PrincipalId), 'employees has member hackers');
ok($employees->HasMemberRecursively($herbert->PrincipalId), 'employees has member herbert');
ok($employees->HasMemberRecursively($eric->PrincipalId), 'employees has member eric');
-ok($hacks->HasMemberRecursively($herbert->PrincipalId), 'hacks has member herbert');
-ok(!$hacks->HasMemberRecursively($eric->PrincipalId), 'hacks does not have member eric');
+ok($hackers->HasMemberRecursively($herbert->PrincipalId), 'hackers has member herbert');
+ok(!$hackers->HasMemberRecursively($eric->PrincipalId), 'hackers does not have member eric');
# There's also a separate group, "Other", which both are a member of.
my $other = RT::Group->new(RT->SystemUser);
@@ -50,76 +50,81 @@ ok($ok, $msg);
ok($ok, $msg);
-{ # Eric doesn't have the right yet
- my $g = RT::Group->new(RT::CurrentUser->new($eric));
- $g->Load($employees->Id);
- ok(!$g->CurrentUserHasRight("RTxGroupRight"), "Eric doesn't yet have the right on employees");
- $g->Load($hacks->Id);
- ok(!$g->CurrentUserHasRight("RTxGroupRight"), "Nor on hackers");
- $g->Load($other->Id);
- ok(!$g->CurrentUserHasRight("RTxGroupRight"), "Nor on other");
+# Everyone can SeeGroup on all three groups
+my $everyone = RT::Group->new( RT->SystemUser );
+($ok, $msg) = $everyone->LoadSystemInternalGroup( 'Everyone' );
+ok($ok, $msg);
+$everyone->PrincipalObj->GrantRight(Right => 'SeeGroup', Object => $employees);
+$everyone->PrincipalObj->GrantRight(Right => 'SeeGroup', Object => $hackers);
+$everyone->PrincipalObj->GrantRight(Right => 'SeeGroup', Object => $other);
+
+sub CheckRights {
+ my $cu = shift;
+ my %groups = (Employees => 0, Hackers => 0, Other => 0, @_);
+ my $name = $cu->Name;
- my $groups = RT::Groups->new(RT::CurrentUser->new($eric));
+ my $groups = RT::Groups->new(RT::CurrentUser->new($cu));
$groups->LimitToUserDefinedGroups;
$groups->ForWhichCurrentUserHasRight(Right => 'RTxGroupRight');
+ my %has_right = map { ($_->Name => 1) } @{ $groups->ItemsArrayRef };
- is_deeply([sort map { $_->Name } @{ $groups->ItemsArrayRef }], [], 'no joined groups have RTxGroupRight yet');
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+ for my $groupname (sort keys %groups) {
+ my $g = RT::Group->new(RT::CurrentUser->new($cu));
+ $g->LoadUserDefinedGroup($groupname);
+ if ($groups{$groupname}) {
+ ok( $g->CurrentUserHasRight("RTxGroupRight"), "$name has right on $groupname (direct query)" );
+ ok( delete $has_right{$groupname}, "..and also in ForWhichCurrentUserHasRight");
+ } else {
+ ok( !$g->CurrentUserHasRight("RTxGroupRight"), "$name doesn't have right on $groupname (direct query)" );
+ ok( !delete $has_right{$groupname}, "..and also not in ForWhichCurrentUserHasRight");
+ }
+ }
+ ok(not(keys %has_right), "ForWhichCurrentUserHasRight has no extra groups");
}
-{ # Neither does Herbert
- my $g = RT::Group->new(RT::CurrentUser->new($herbert));
- $g->Load($employees->Id);
- ok(!$g->CurrentUserHasRight("RTxGroupRight"), "Herbert doesn't yet have the right on employees");
- $g->Load($hacks->Id);
- ok(!$g->CurrentUserHasRight("RTxGroupRight"), "Nor on hackers");
- $g->Load($other->Id);
- ok(!$g->CurrentUserHasRight("RTxGroupRight"), "Nor on other");
+# Neither should have it on any group yet
+CheckRights($eric);
+CheckRights($herbert);
- my $groups = RT::Groups->new(RT::CurrentUser->new($herbert));
- $groups->LimitToUserDefinedGroups;
- $groups->ForWhichCurrentUserHasRight(Right => 'RTxGroupRight');
- is_deeply([sort map { $_->Name } @{ $groups->ItemsArrayRef }], [], 'no joined groups have RTxGroupRight yet');
-}
-# Grant it to employees, on employees
-($ok, $msg) = $employees->PrincipalObj->GrantRight(Right => 'RTxGroupRight', Object => $employees);
-ok($ok, $msg);
+# Grant it to employees, on employees. Both Herbert and Eric will have
+# it on employees, though Herbert gets it by way of hackers. Neither
+# will have it on hackers, because the target does not recurse.
+$employees->PrincipalObj->GrantRight( Right => 'RTxGroupRight', Object => $employees);
+CheckRights($eric, Employees => 1);
+CheckRights($herbert, Employees => 1);
-{ # Eric is an Employee directly, so has it on Employees, but not on Hack or Other
- my $g = RT::Group->new(RT::CurrentUser->new($eric));
- $g->Load($employees->Id);
- ok($g->CurrentUserHasRight("RTxGroupRight"), "Eric has the right on employees");
- $g->Load($hacks->Id);
- ok(!$g->CurrentUserHasRight("RTxGroupRight"), "Doesn't have it on hackers");
- $g->Load($other->Id);
- ok(!$g->CurrentUserHasRight("RTxGroupRight"), "Nor on other");
- my $groups = RT::Groups->new(RT::CurrentUser->new($eric));
- $groups->LimitToUserDefinedGroups;
- $groups->ForWhichCurrentUserHasRight(Right => 'RTxGroupRight');
- my %has_right = map { ($_->Name => 1) } @{ $groups->ItemsArrayRef };
- ok(delete $has_right{Employees}, "Has the right on a group it's in");
- ok(not(delete $has_right{Hackers}), "Doesn't have the right on a group it's not in");
- ok(not(delete $has_right{Other}), "Doesn't have the right on a group it's in");
- ok(not(keys %has_right), "Has no other groups")
-}
+# Grant it to employees, on hackers. This means both Eric and Herbert
+# will have the right on hackers, but not on employees.
+$employees->PrincipalObj->RevokeRight(Right => 'RTxGroupRight', Object => $employees);
+$employees->PrincipalObj->GrantRight( Right => 'RTxGroupRight', Object => $hackers);
+CheckRights($eric, Hackers => 1);
+CheckRights($herbert, Hackers => 1);
-{ # Herbert is an Employee by way of Hackers, so should have it on both
- my $g = RT::Group->new(RT::CurrentUser->new($herbert));
- $g->Load($employees->Id);
- ok($g->CurrentUserHasRight("RTxGroupRight"), "Herbert has the right on employees");
- $g->Load($hacks->Id);
- ok(!$g->CurrentUserHasRight("RTxGroupRight"), "Also has it on hackers");
- $g->Load($other->Id);
- ok(!$g->CurrentUserHasRight("RTxGroupRight"), "But not on other");
- my $groups = RT::Groups->new(RT::CurrentUser->new($herbert));
- $groups->LimitToUserDefinedGroups;
- $groups->ForWhichCurrentUserHasRight(Right => 'RTxGroupRight');
+# Grant it to hackers, on employees. Eric will have it nowhere, and
+# Herbert will have it on employees. Note that the target of the right
+# itself does _not_ recurse down, so Herbert will not have it on
+# hackers.
+$employees->PrincipalObj->RevokeRight(Right => 'RTxGroupRight', Object => $hackers);
+$hackers->PrincipalObj->GrantRight( Right => 'RTxGroupRight', Object => $employees);
+CheckRights($eric);
+CheckRights($herbert, Employees => 1);
- my %has_right = map { ($_->Name => 1) } @{ $groups->ItemsArrayRef };
- ok(delete $has_right{Employees}, "Has the right on a group it's in");
- ok(delete $has_right{Hackers}, "Grants the right recursively");
- ok(not(delete $has_right{Other}), "Doesn't have the right on a group it's in");
- ok(not(keys %has_right), "Has no other groups")
-}
+
+# Grant it globally to hackers; herbert will see the right on all
+# employees, hackers, and other.
+$hackers->PrincipalObj->RevokeRight( Right => 'RTxGroupRight', Object => $employees);
+$hackers->PrincipalObj->GrantRight( Right => 'RTxGroupRight', Object => RT->System);
+CheckRights($eric);
+CheckRights($herbert, Employees => 1, Hackers => 1, Other => 1 );
+
+
+# Grant it globally to employees; both eric and herbert will see the
+# right on all employees, hackers, and other.
+$hackers->PrincipalObj->RevokeRight( Right => 'RTxGroupRight', Object => RT->System);
+$employees->PrincipalObj->GrantRight( Right => 'RTxGroupRight', Object => RT->System);
+CheckRights($eric, Employees => 1, Hackers => 1, Other => 1 );
+CheckRights($herbert, Employees => 1, Hackers => 1, Other => 1 );
diff --git a/t/web/dashboards-groups.t b/t/web/dashboards-groups.t
index ba64cc6..382dd12 100644
--- a/t/web/dashboards-groups.t
+++ b/t/web/dashboards-groups.t
@@ -98,19 +98,13 @@ $m->content_contains('Subscription', "Subscription link not hidden because we ha
$m->get_ok("/Dashboards/index.html");
-TODO: {
- local $TODO = "We currently entirely fail to show group dashboards";
- $m->content_contains("inner dashboard", "We can see the inner dashboard from the UI");
-}
+$m->content_contains("inner dashboard", "We can see the inner dashboard from the UI");
my ($group) = grep {$_->isa("RT::Group") and $_->Id == $inner_group->Id}
RT::Dashboard->new($currentuser)->_PrivacyObjects;
ok($group, "Found the group in the privacy objects list");
-TODO: {
- local $TODO = "We currently entirely fail to show group dashboards";
- ($group) = grep {$_->isa("RT::Group") and $_->Id == $inner_group->Id}
- RT::Dashboard->new($currentuser)->ObjectsForLoading;
- ok($group, "Found the group in the objects for loading");
-}
+($group) = grep {$_->isa("RT::Group") and $_->Id == $inner_group->Id}
+ RT::Dashboard->new($currentuser)->ObjectsForLoading;
+ok($group, "Found the group in the objects for loading");
commit 0707c1866ed8b1556ab29a5abd9b2668302d0788
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Jul 14 19:44:12 2011 -0400
Add back prevention for "used only once" warnings, removed in the split in 47b1c48
diff --git a/t/api/groups.t b/t/api/groups.t
index e368283..d2dc126 100644
--- a/t/api/groups.t
+++ b/t/api/groups.t
@@ -39,7 +39,7 @@ RT::Group->AddRights(
is ($groups->First->Id , $g->Id, "it's the right one");
}
-no warnings qw/redefine/;
+no warnings qw/redefine once/;
my $q = RT::Queue->new(RT->SystemUser);
my ($id, $msg) =$q->Create( Name => 'GlobalACLTest');
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list