Duplicate code is one of the most pungent code smells. A rule that is often used is to re-structure code once it is duplicated in three or more places.
Common duplication problems, and corresponding solutions are:
1 | <?php |
||
15 | final class SecurityManager |
||
16 | { |
||
17 | const ALLOWED = 1; |
||
18 | const ERROR_NOT_IDENTIFIED = 2; |
||
19 | const ERROR_DENIED = 3; |
||
20 | /** @var IdentificationVerifier */ |
||
21 | private $identificationVerifier; |
||
22 | /** |
||
23 | * @var RoleConfiguration |
||
24 | */ |
||
25 | private $roleConfiguration; |
||
26 | |||
27 | /** |
||
28 | * SecurityManager constructor. |
||
29 | * |
||
30 | * @param IdentificationVerifier $identificationVerifier |
||
31 | * @param RoleConfiguration $roleConfiguration |
||
32 | */ |
||
33 | public function __construct( |
||
34 | IdentificationVerifier $identificationVerifier, |
||
35 | RoleConfiguration $roleConfiguration |
||
36 | ) { |
||
37 | $this->identificationVerifier = $identificationVerifier; |
||
38 | $this->roleConfiguration = $roleConfiguration; |
||
39 | } |
||
40 | |||
41 | /** |
||
42 | * Tests if a user is allowed to perform an action. |
||
43 | * |
||
44 | * This method should form a hard, deterministic security barrier, and only return true if it is absolutely sure |
||
45 | * that a user should have access to something. |
||
46 | * |
||
47 | * @param string $page |
||
48 | * @param string $route |
||
49 | * @param User $user |
||
50 | * |
||
51 | * @return int |
||
52 | * |
||
53 | * @category Security-Critical |
||
54 | */ |
||
55 | public function allows($page, $route, User $user) |
||
56 | { |
||
57 | $this->getActiveRoles($user, $activeRoles, $inactiveRoles); |
||
58 | |||
59 | $availableRights = $this->flattenRoles($activeRoles); |
||
60 | $testResult = $this->findResult($availableRights, $page, $route); |
||
61 | |||
62 | if ($testResult !== null) { |
||
63 | // We got a firm result here, so just return it. |
||
64 | return $testResult; |
||
65 | } |
||
66 | |||
67 | // No firm result yet, so continue testing the inactive roles so we can give a better error. |
||
68 | $inactiveRights = $this->flattenRoles($inactiveRoles); |
||
69 | $testResult = $this->findResult($inactiveRights, $page, $route); |
||
70 | |||
71 | if ($testResult === self::ALLOWED) { |
||
72 | // The user is allowed to access this, but their role is inactive. |
||
73 | return self::ERROR_NOT_IDENTIFIED; |
||
74 | } |
||
75 | |||
76 | // Other options from the secondary test are denied and inconclusive, which at this point defaults to denied. |
||
77 | return self::ERROR_DENIED; |
||
78 | } |
||
79 | |||
80 | /** |
||
81 | * @param array $pseudoRole The role (flattened) to check |
||
82 | * @param string $page The page class to check |
||
83 | * @param string $route The page route to check |
||
84 | * |
||
85 | * @return int|null |
||
86 | */ |
||
87 | private function findResult($pseudoRole, $page, $route) |
||
88 | { |
||
89 | if (isset($pseudoRole[$page])) { |
||
90 | // check for deny on catch-all route |
||
91 | View Code Duplication | if (isset($pseudoRole[$page][RoleConfiguration::ALL])) { |
|
|
|||
92 | if ($pseudoRole[$page][RoleConfiguration::ALL] === RoleConfiguration::ACCESS_DENY) { |
||
93 | return self::ERROR_DENIED; |
||
94 | } |
||
95 | } |
||
96 | |||
97 | // check normal route |
||
98 | if (isset($pseudoRole[$page][$route])) { |
||
99 | if ($pseudoRole[$page][$route] === RoleConfiguration::ACCESS_DENY) { |
||
100 | return self::ERROR_DENIED; |
||
101 | } |
||
102 | |||
103 | if ($pseudoRole[$page][$route] === RoleConfiguration::ACCESS_ALLOW) { |
||
104 | return self::ALLOWED; |
||
105 | } |
||
106 | } |
||
107 | |||
108 | // check for allowed on catch-all route |
||
109 | View Code Duplication | if (isset($pseudoRole[$page][RoleConfiguration::ALL])) { |
|
110 | if ($pseudoRole[$page][RoleConfiguration::ALL] === RoleConfiguration::ACCESS_ALLOW) { |
||
111 | return self::ALLOWED; |
||
112 | } |
||
113 | } |
||
114 | } |
||
115 | |||
116 | // return indeterminate result |
||
117 | return null; |
||
118 | } |
||
119 | |||
120 | /** |
||
121 | * Takes an array of roles and flattens the values to a single set. |
||
122 | * |
||
123 | * @param array $activeRoles |
||
124 | * |
||
125 | * @return array |
||
126 | */ |
||
127 | private function flattenRoles($activeRoles) |
||
161 | |||
162 | /** |
||
163 | * @param User $user |
||
164 | * @param array $activeRoles |
||
165 | * @param array $inactiveRoles |
||
166 | */ |
||
167 | public function getActiveRoles(User $user, &$activeRoles, &$inactiveRoles) |
||
209 | |||
210 | public function getRoleConfiguration(){ |
||
213 | } |
||
214 |
Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.
You can also find more detailed suggestions in the “Code” section of your repository.