Skip to content

Commit b924d45

Browse files
krulis-martinkrulis-martin
committed
Optimizing ACL policies with caching of group-membership check results.
1 parent 4155f3e commit b924d45

13 files changed

+152
-118
lines changed

‎app/V1Module/security/Policies/AssignmentPermissionPolicy.php‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,12 @@
33
namespace App\Security\Policies;
44

55
use App\Model\Entity\Assignment;
6+
use App\Model\Entity\GroupMembership;
67
use App\Security\Identity;
78
use App\Helpers\SubmissionConfigHelper;
89
use DateTime;
910

10-
class AssignmentPermissionPolicy implements IPermissionPolicy
11+
class AssignmentPermissionPolicy extends BasePermissionPolicy implements IPermissionPolicy
1112
{
1213
/** @var SubmissionConfigHelper */
1314
private $submissionHelper;
@@ -70,26 +71,20 @@ public function isAssignee(Identity $identity, Assignment $assignment)
7071

7172
public function isSupervisorOrAdmin(Identity $identity, Assignment $assignment)
7273
{
73-
$group = $assignment->getGroup();
74-
$user = $identity->getUserData();
75-
76-
if ($user === null) {
77-
return false;
78-
}
79-
80-
return $group && ($group->isSupervisorOf($user) || $group->isAdminOf($user));
74+
return $this->checkMinimalMembership(
75+
$identity->getUserData(),
76+
$assignment->getGroup(),
77+
GroupMembership::TYPE_SUPERVISOR
78+
);
8179
}
8280

8381
public function isObserverOrBetter(Identity $identity, Assignment $assignment)
8482
{
85-
$group = $assignment->getGroup();
86-
$user = $identity->getUserData();
87-
88-
if ($user === null) {
89-
return false;
90-
}
91-
92-
return $group && ($group->isObserverOf($user) || $group->isSupervisorOf($user) || $group->isAdminOf($user));
83+
return $this->checkMinimalMembership(
84+
$identity->getUserData(),
85+
$assignment->getGroup(),
86+
GroupMembership::TYPE_OBSERVER
87+
);
9388
}
9489

9590
/**

‎app/V1Module/security/Policies/AssignmentSolutionPermissionPolicy.php‎

Lines changed: 17 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@
33
namespace App\Security\Policies;
44

55
use App\Model\Entity\AssignmentSolution;
6-
use App\Model\Entity\AssignmentSolutionSubmission;
6+
use App\Model\Entity\GroupMembership;
77
use App\Security\Identity;
88

9-
class AssignmentSolutionPermissionPolicy implements IPermissionPolicy
9+
class AssignmentSolutionPermissionPolicy extends BasePermissionPolicy implements IPermissionPolicy
1010
{
1111
public function getAssociatedClass()
1212
{
@@ -20,14 +20,11 @@ public function isAdmin(Identity $identity, AssignmentSolution $solution)
2020
return false;
2121
}
2222

23-
$group = $assignment->getGroup();
24-
$user = $identity->getUserData();
25-
26-
if ($user === null) {
27-
return false;
28-
}
29-
30-
return $group && $group->isAdminOf($user);
23+
return $this->checkMinimalMembership(
24+
$identity->getUserData(),
25+
$assignment->getGroup(),
26+
GroupMembership::TYPE_ADMIN
27+
);
3128
}
3229

3330
public function isSupervisorOrAdmin(Identity $identity, AssignmentSolution $solution)
@@ -37,14 +34,11 @@ public function isSupervisorOrAdmin(Identity $identity, AssignmentSolution $solu
3734
return false;
3835
}
3936

40-
$group = $assignment->getGroup();
41-
$user = $identity->getUserData();
42-
43-
if ($user === null) {
44-
return false;
45-
}
46-
47-
return $group && ($group->isSupervisorOf($user) || $group->isAdminOf($user));
37+
return $this->checkMinimalMembership(
38+
$identity->getUserData(),
39+
$assignment->getGroup(),
40+
GroupMembership::TYPE_SUPERVISOR
41+
);
4842
}
4943

5044
public function isObserverOrBetter(Identity $identity, AssignmentSolution $solution)
@@ -54,14 +48,11 @@ public function isObserverOrBetter(Identity $identity, AssignmentSolution $solut
5448
return false;
5549
}
5650

57-
$group = $assignment->getGroup();
58-
$user = $identity->getUserData();
59-
60-
if ($user === null) {
61-
return false;
62-
}
63-
64-
return $group && ($group->isObserverOf($user) || $group->isSupervisorOf($user) || $group->isAdminOf($user));
51+
return $this->checkMinimalMembership(
52+
$identity->getUserData(),
53+
$assignment->getGroup(),
54+
GroupMembership::TYPE_OBSERVER
55+
);
6556
}
6657

6758
public function isAuthor(Identity $identity, AssignmentSolution $solution)

‎app/V1Module/security/Policies/BasePermissionPolicy.php‎

Lines changed: 49 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,25 +2,64 @@
22

33
namespace App\Security\Policies;
44

5-
use App\Model\Entity\Instance;
5+
use App\Model\Entity\GroupMembership;
6+
use App\Model\Entity\Group;
67
use App\Model\Entity\User;
7-
use App\Security\Identity;
88
use App\Security\Roles;
9+
use InvalidArgumentException;
910

1011
/**
11-
* Base policy is not bound to particular entity.
12-
* It gathers generic checks performed solely on the entity of the logged-in user.
12+
* Base policy class that implements caching mechanisms reusable by other policies.
1313
*/
14-
class BasePermissionPolicy implements IPermissionPolicy
14+
class BasePermissionPolicy
1515
{
16-
public function getAssociatedClass()
16+
private const MEMBERSHIPS = [
17+
GroupMembership::TYPE_STUDENT => 1,
18+
GroupMembership::TYPE_OBSERVER => 2,
19+
GroupMembership::TYPE_SUPERVISOR => 3,
20+
GroupMembership::TYPE_ADMIN => 4,
21+
];
22+
23+
private static array $membershipCache = [];
24+
25+
protected function getMembershipLevel(User $user, Group $group): int
1726
{
18-
return '';
27+
$gid = $group->getId();
28+
if (!array_key_exists($gid, self::$membershipCache)) {
29+
self::$membershipCache[$gid] = 0; // Not a member
30+
31+
if ($user->getRole() === Roles::STUDENT_ROLE) {
32+
if ($group->isStudentOf($user)) {
33+
self::$membershipCache[$gid] = self::MEMBERSHIPS[GroupMembership::TYPE_STUDENT];
34+
}
35+
} else {
36+
if ($group->isAdminOf($user)) {
37+
self::$membershipCache[$gid] = self::MEMBERSHIPS[GroupMembership::TYPE_ADMIN];
38+
} elseif ($group->isSupervisorOf($user)) {
39+
self::$membershipCache[$gid] = self::MEMBERSHIPS[GroupMembership::TYPE_SUPERVISOR];
40+
} elseif ($group->isObserverOf($user)) {
41+
self::$membershipCache[$gid] = self::MEMBERSHIPS[GroupMembership::TYPE_OBSERVER];
42+
} elseif ($user->getRole() === Roles::STUDENT_ROLE && $group->isStudentOf($user)) {
43+
self::$membershipCache[$gid] = self::MEMBERSHIPS[GroupMembership::TYPE_STUDENT];
44+
}
45+
}
46+
}
47+
48+
return self::$membershipCache[$gid];
1949
}
2050

21-
public function userIsNotGroupLocked(Identity $identity): bool
51+
protected function checkMinimalMembership(?User $user, ?Group $group, string $membership): bool
2252
{
23-
$user = $identity->getUserData();
24-
return $user && !$user->isGroupLocked();
53+
if (!$user || !$group) {
54+
return false;
55+
}
56+
57+
$minLevel = self::MEMBERSHIPS[$membership] ?? null;
58+
if ($minLevel === null) {
59+
throw new InvalidArgumentException("Unknown membership type: $membership");
60+
}
61+
62+
$level = $this->getMembershipLevel($user, $group);
63+
return $level >= $minLevel;
2564
}
2665
}

‎app/V1Module/security/Policies/CommentPermissionPolicy.php‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,12 @@
55
use App\Model\Entity\Assignment;
66
use App\Model\Entity\AssignmentSolution;
77
use App\Model\Entity\Comment;
8+
use App\Model\Entity\GroupMembership;
89
use App\Model\Repository\Assignments;
910
use App\Model\Repository\AssignmentSolutions;
1011
use App\Security\Identity;
1112

12-
class CommentPermissionPolicy implements IPermissionPolicy
13+
class CommentPermissionPolicy extends BasePermissionPolicy implements IPermissionPolicy
1314
{
1415
private $assignments;
1516
private $assignmentSolutions;
@@ -62,8 +63,11 @@ public function isSupervisorInGroupOfCommentedSolution(Identity $identity, Comme
6263
return false;
6364
}
6465

65-
$group = $solution->getAssignment()->getGroup();
66-
return $group && ($group->isSupervisorOf($user) || $group->isAdminOf($user));
66+
return $this->checkMinimalMembership(
67+
$user,
68+
$solution->getAssignment()->getGroup(),
69+
GroupMembership::TYPE_SUPERVISOR
70+
);
6771
}
6872

6973

@@ -80,7 +84,6 @@ public function isSupervisorInGroupOfCommentedAssignment(Identity $identity, Com
8084
return false;
8185
}
8286

83-
$group = $assignment->getGroup();
84-
return $group && ($group->isSupervisorOf($user) || $group->isAdminOf($user));
87+
return $this->checkMinimalMembership($user, $assignment->getGroup(), GroupMembership::TYPE_SUPERVISOR);
8588
}
8689
}

‎app/V1Module/security/Policies/ExercisePermissionPolicy.php‎

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,11 @@
44

55
use App\Model\Entity\Exercise;
66
use App\Model\Entity\Group;
7+
use App\Model\Entity\GroupMembership;
78
use App\Helpers\SubmissionConfigHelper;
89
use App\Security\Identity;
910

10-
class ExercisePermissionPolicy implements IPermissionPolicy
11+
class ExercisePermissionPolicy extends BasePermissionPolicy implements IPermissionPolicy
1112
{
1213
/** @var SubmissionConfigHelper */
1314
private $submissionHelper;
@@ -121,15 +122,6 @@ public function isSubGroupNonStudentMember(Identity $identity, Exercise $exercis
121122
return false;
122123
}
123124

124-
/**
125-
* @var Array[]
126-
* A cache holding the result of isAdminOf invocation for given groups.
127-
* The cache is structures as [user-id][group-id] => boolean
128-
* Under normal circumstances, the cache should hold only one (logged in) user,
129-
* but it was written as generic cache just in case.
130-
*/
131-
private $supergroupAdminCache = [];
132-
133125
public function isSuperGroupAdmin(Identity $identity, Exercise $exercise)
134126
{
135127
$user = $identity->getUserData();
@@ -141,21 +133,12 @@ public function isSuperGroupAdmin(Identity $identity, Exercise $exercise)
141133
return false;
142134
}
143135

144-
if (empty($this->supergroupAdminCache[$user->getId()])) {
145-
$this->supergroupAdminCache[$user->getId()] = [];
146-
}
147-
$supergroupCache = &$this->supergroupAdminCache[$user->getId()];
148-
149136
/** @var Group $group */
150137
foreach ($exercise->getGroups() as $group) {
151-
if (!array_key_exists($group->getId(), $supergroupCache)) {
152-
$supergroupCache[$group->getId()] = $group->isAdminOf($user);
153-
}
154-
if ($supergroupCache[$group->getId()]) {
138+
if ($this->checkMinimalMembership($user, $group, GroupMembership::TYPE_ADMIN)) {
155139
return true;
156140
}
157141
}
158-
159142
return false;
160143
}
161144

‎app/V1Module/security/Policies/GenericPermissionPolicy.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php
2+
3+
namespace App\Security\Policies;
4+
5+
use App\Security\Identity;
6+
7+
/**
8+
* A policy that is not bound to a particular entity.
9+
* It gathers generic checks performed solely on the entity of the logged-in user.
10+
*/
11+
class GenericPermissionPolicy implements IPermissionPolicy
12+
{
13+
public function getAssociatedClass()
14+
{
15+
return '';
16+
}
17+
18+
public function userIsNotGroupLocked(Identity $identity): bool
19+
{
20+
$user = $identity->getUserData();
21+
return $user && !$user->isGroupLocked();
22+
}
23+
}

‎app/V1Module/security/Policies/GroupPermissionPolicy.php‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,12 @@
33
namespace App\Security\Policies;
44

55
use App\Model\Entity\Group;
6+
use App\Model\Entity\GroupMembership;
67
use App\Model\Entity\Instance;
78
use App\Security\Identity;
89
use DateTIme;
910

10-
class GroupPermissionPolicy implements IPermissionPolicy
11+
class GroupPermissionPolicy extends BasePermissionPolicy implements IPermissionPolicy
1112
{
1213
public function getAssociatedClass()
1314
{
@@ -21,17 +22,20 @@ public function isMember(Identity $identity, Group $group): bool
2122
return false;
2223
}
2324

24-
return $group->isMemberOf($user) || $group->isAdminOf($user);
25+
return $group->isMemberOf($user) || $this->checkMinimalMembership(
26+
$identity->getUserData(),
27+
$group,
28+
GroupMembership::TYPE_ADMIN
29+
);
2530
}
2631

2732
public function isSupervisorOrAdmin(Identity $identity, Group $group): bool
2833
{
29-
$user = $identity->getUserData();
30-
if (!$user) {
31-
return false;
32-
}
33-
34-
return $group->isSupervisorOf($user) || $group->isAdminOf($user);
34+
return $this->checkMinimalMembership(
35+
$identity->getUserData(),
36+
$group,
37+
GroupMembership::TYPE_SUPERVISOR
38+
);
3539
}
3640

3741
public function isObserver(Identity $identity, Group $group): bool
@@ -46,12 +50,11 @@ public function isObserver(Identity $identity, Group $group): bool
4650

4751
public function isAdmin(Identity $identity, Group $group): bool
4852
{
49-
$user = $identity->getUserData();
50-
if (!$user) {
51-
return false;
52-
}
53-
54-
return $group->isAdminOf($user);
53+
return $this->checkMinimalMembership(
54+
$identity->getUserData(),
55+
$group,
56+
GroupMembership::TYPE_ADMIN
57+
);
5558
}
5659

5760
public function isPublic(Identity $identity, Group $group): bool

‎app/V1Module/security/Policies/NotificationPermissionPolicy.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,12 @@
33
namespace App\Security\Policies;
44

55
use App\Model\Entity\Group;
6+
use App\Model\Entity\GroupMembership;
67
use App\Model\Entity\Notification;
78
use App\Security\Identity;
89
use App\Security\Roles;
910

10-
class NotificationPermissionPolicy implements IPermissionPolicy
11+
class NotificationPermissionPolicy extends BasePermissionPolicy implements IPermissionPolicy
1112
{
1213
/** @var Roles */
1314
private $roles;
@@ -57,7 +58,7 @@ public function isAncestorGroupAdmin(Identity $identity, Notification $notificat
5758

5859
/** @var Group $group */
5960
foreach ($notification->getGroups() as $group) {
60-
if ($group->isAdminOf($user)) {
61+
if ($this->checkMinimalMembership($user, $group, GroupMembership::TYPE_ADMIN)) {
6162
return true;
6263
}
6364
}

0 commit comments

Comments
 (0)