owncloud /
core
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:
Complex classes like USER_LDAP often do a lot of different things. To break such a class down, we need to identify a cohesive component within that class. A common approach to find such a component is to look for fields/methods that share the same prefixes, or suffixes. You can also have a look at the cohesion graph to spot any un-connected, or weakly-connected components.
Once you have determined the fields that belong together, you can apply the Extract Class refactoring. If the component makes sense as a sub-class, Extract Subclass is also a candidate, and is often faster.
While breaking up the class, it is a good idea to analyze how other classes use USER_LDAP, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 40 | class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserInterface { |
||
| 41 | /** @var string[] $homesToKill */ |
||
| 42 | protected $homesToKill = array(); |
||
| 43 | |||
| 44 | /** @var \OCP\IConfig */ |
||
| 45 | protected $ocConfig; |
||
| 46 | |||
| 47 | /** |
||
| 48 | * @param \OCA\user_ldap\lib\Access $access |
||
| 49 | * @param \OCP\IConfig $ocConfig |
||
| 50 | */ |
||
| 51 | 34 | public function __construct(Access $access, IConfig $ocConfig) { |
|
| 55 | |||
| 56 | /** |
||
| 57 | * checks whether the user is allowed to change his avatar in ownCloud |
||
| 58 | * @param string $uid the ownCloud user name |
||
| 59 | * @return boolean either the user can or cannot |
||
| 60 | */ |
||
| 61 | public function canChangeAvatar($uid) { |
||
| 72 | |||
| 73 | /** |
||
| 74 | * returns the username for the given login name, if available |
||
| 75 | * |
||
| 76 | * @param string $loginName |
||
| 77 | * @return string|false |
||
| 78 | */ |
||
| 79 | public function loginName2UserName($loginName) { |
||
| 91 | |||
| 92 | /** |
||
| 93 | * returns an LDAP record based on a given login name |
||
| 94 | * |
||
| 95 | * @param string $loginName |
||
| 96 | * @return array |
||
| 97 | * @throws \Exception |
||
| 98 | */ |
||
| 99 | 7 | public function getLDAPUserByLoginName($loginName) { |
|
| 108 | |||
| 109 | /** |
||
| 110 | * Check if the password is correct |
||
| 111 | * @param string $uid The username |
||
| 112 | * @param string $password The password |
||
| 113 | * @return false|string |
||
| 114 | * |
||
| 115 | * Check if the password is correct without logging in the user |
||
| 116 | */ |
||
| 117 | 17 | public function checkPassword($uid, $password) { |
|
| 118 | try { |
||
| 119 | 7 | $ldapRecord = $this->getLDAPUserByLoginName($uid); |
|
| 120 | 7 | } catch(\Exception $e) { |
|
| 121 | 2 | \OC::$server->getLogger()->logException($e, ['app' => 'user_ldap']); |
|
| 122 | 2 | return false; |
|
| 123 | } |
||
| 124 | 5 | $dn = $ldapRecord['dn'][0]; |
|
| 125 | 5 | $user = $this->access->userManager->get($dn); |
|
| 126 | |||
| 127 | 5 | if(!$user instanceof User) { |
|
| 128 | 1 | \OCP\Util::writeLog('user_ldap', |
|
| 129 | 1 | 'LDAP Login: Could not get user object for DN ' . $dn . |
|
| 130 | 1 | '. Maybe the LDAP entry has no set display name attribute?', |
|
| 131 | 1 | \OCP\Util::WARN); |
|
| 132 | 1 | return false; |
|
| 133 | } |
||
| 134 | 4 | if($user->getUsername() !== false) { |
|
| 135 | //are the credentials OK? |
||
| 136 | 4 | if(!$this->access->areCredentialsValid($dn, $password)) { |
|
| 137 | 2 | return false; |
|
| 138 | } |
||
| 139 | |||
| 140 | 17 | $this->access->cacheUserExists($user->getUsername()); |
|
| 141 | 17 | $user->processAttributes($ldapRecord); |
|
| 142 | 17 | $user->markLogin(); |
|
| 143 | |||
| 144 | 2 | return $user->getUsername(); |
|
| 145 | } |
||
| 146 | |||
| 147 | return false; |
||
| 148 | 2 | } |
|
| 149 | |||
| 150 | /** |
||
| 151 | * Get a list of all users |
||
| 152 | * |
||
| 153 | * @param string $search |
||
| 154 | * @param null|int $limit |
||
| 155 | * @param null|int $offset |
||
| 156 | * @return string[] an array of all uids |
||
| 157 | */ |
||
| 158 | 12 | public function getUsers($search = '', $limit = 10, $offset = 0) { |
|
| 198 | |||
| 199 | /** |
||
| 200 | * checks whether a user is still available on LDAP |
||
| 201 | * |
||
| 202 | * @param string|\OCA\User_LDAP\lib\user\User $user either the ownCloud user |
||
| 203 | * name or an instance of that user |
||
| 204 | * @return bool |
||
| 205 | * @throws \Exception |
||
| 206 | * @throws \OC\ServerNotAvailableException |
||
| 207 | 10 | */ |
|
| 208 | 10 | public function userExistsOnLDAP($user) { |
|
| 248 | |||
| 249 | 12 | /** |
|
| 250 | 12 | * check if a user exists |
|
| 251 | * @param string $uid the username |
||
| 252 | * @return boolean |
||
| 253 | * @throws \Exception when connection could not be established |
||
| 254 | 12 | */ |
|
| 255 | public function userExists($uid) { |
||
| 281 | 3 | ||
| 282 | 3 | /** |
|
| 283 | 3 | * returns whether a user was deleted in LDAP |
|
| 284 | 2 | * |
|
| 285 | 2 | * @param string $uid The username of the user to delete |
|
| 286 | 2 | * @return bool |
|
| 287 | 2 | */ |
|
| 288 | public function deleteUser($uid) { |
||
| 307 | |||
| 308 | /** |
||
| 309 | 5 | * get the user's home directory |
|
| 310 | 5 | * |
|
| 311 | * @param string $uid the username |
||
| 312 | 1 | * @return bool|string |
|
| 313 | * @throws NoUserException |
||
| 314 | * @throws \Exception |
||
| 315 | */ |
||
| 316 | 4 | public function getHome($uid) { |
|
| 348 | 1 | ||
| 349 | /** |
||
| 350 | * get display name of the user |
||
| 351 | 2 | * @param string $uid user ID of the user |
|
| 352 | 2 | * @return string|false display name |
|
| 353 | */ |
||
| 354 | public function getDisplayName($uid) { |
||
| 392 | |||
| 393 | /** |
||
| 394 | * Get a list of all display names |
||
| 395 | * |
||
| 396 | * @param string $search |
||
| 397 | * @param string|null $limit |
||
| 398 | * @param string|null $offset |
||
| 399 | * @return array an array of all displayNames (value) and the corresponding uids (key) |
||
| 400 | */ |
||
| 401 | public function getDisplayNames($search = '', $limit = null, $offset = null) { |
||
| 415 | |||
| 416 | 4 | /** |
|
| 417 | * Check if backend implements actions |
||
| 418 | 4 | * @param int $actions bitwise-or'ed actions |
|
| 419 | 4 | * @return boolean |
|
| 420 | 4 | * |
|
| 421 | 4 | * Returns the supported actions as int to be |
|
| 422 | 4 | * compared with OC_USER_BACKEND_CREATE_USER etc. |
|
| 423 | */ |
||
| 424 | public function implementsActions($actions) { |
||
| 432 | |||
| 433 | /** |
||
| 434 | * @return bool |
||
| 435 | */ |
||
| 436 | public function hasUserListings() { |
||
| 439 | 2 | ||
| 440 | 2 | /** |
|
| 441 | * counts the users in LDAP |
||
| 442 | * |
||
| 443 | 2 | * @return int|bool |
|
| 444 | 2 | */ |
|
| 445 | 2 | public function countUsers() { |
|
| 455 | |||
| 456 | /** |
||
| 457 | * Backend name to be shown in user management |
||
| 458 | * @return string the name of the backend to be shown |
||
| 459 | */ |
||
| 460 | public function getBackendName(){ |
||
| 463 | |||
| 464 | } |
||
| 465 |
Loading history...
This check compares calls to functions or methods with their respective definitions. If the call has more arguments than are defined, it raises an issue.
If a function is defined several times with a different number of parameters, the check may pick up the wrong definition and report false positives. One codebase where this has been known to happen is Wordpress.
In this case you can add the
@ignorePhpDoc annotation to the duplicate definition and it will be ignored.