nextcloud /
server
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 UsersController 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 UsersController, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 52 | class UsersController extends OCSController { |
||
| 53 | |||
| 54 | /** @var IUserManager */ |
||
| 55 | private $userManager; |
||
| 56 | /** @var IConfig */ |
||
| 57 | private $config; |
||
| 58 | /** @var IAppManager */ |
||
| 59 | private $appManager; |
||
| 60 | /** @var IGroupManager|\OC\Group\Manager */ // FIXME Requires a method that is not on the interface |
||
| 61 | private $groupManager; |
||
| 62 | /** @var IUserSession */ |
||
| 63 | private $userSession; |
||
| 64 | /** @var AccountManager */ |
||
| 65 | private $accountManager; |
||
| 66 | /** @var ILogger */ |
||
| 67 | private $logger; |
||
| 68 | /** @var IFactory */ |
||
| 69 | private $l10nFactory; |
||
| 70 | /** @var NewUserMailHelper */ |
||
| 71 | private $newUserMailHelper; |
||
| 72 | /** @var FederatedFileSharingFactory */ |
||
| 73 | private $federatedFileSharingFactory; |
||
| 74 | |||
| 75 | /** |
||
| 76 | * @param string $appName |
||
| 77 | * @param IRequest $request |
||
| 78 | * @param IUserManager $userManager |
||
| 79 | * @param IConfig $config |
||
| 80 | * @param IAppManager $appManager |
||
| 81 | * @param IGroupManager $groupManager |
||
| 82 | * @param IUserSession $userSession |
||
| 83 | * @param AccountManager $accountManager |
||
| 84 | * @param ILogger $logger |
||
| 85 | * @param IFactory $l10nFactory |
||
| 86 | * @param NewUserMailHelper $newUserMailHelper |
||
| 87 | * @param FederatedFileSharingFactory $federatedFileSharingFactory |
||
| 88 | */ |
||
| 89 | View Code Duplication | public function __construct($appName, |
|
| 90 | IRequest $request, |
||
| 91 | IUserManager $userManager, |
||
| 92 | IConfig $config, |
||
| 93 | IAppManager $appManager, |
||
| 94 | IGroupManager $groupManager, |
||
| 95 | IUserSession $userSession, |
||
| 96 | AccountManager $accountManager, |
||
| 97 | ILogger $logger, |
||
| 98 | IFactory $l10nFactory, |
||
| 99 | NewUserMailHelper $newUserMailHelper, |
||
| 100 | FederatedFileSharingFactory $federatedFileSharingFactory) { |
||
| 101 | parent::__construct($appName, $request); |
||
| 102 | |||
| 103 | $this->userManager = $userManager; |
||
| 104 | $this->config = $config; |
||
| 105 | $this->appManager = $appManager; |
||
| 106 | $this->groupManager = $groupManager; |
||
| 107 | $this->userSession = $userSession; |
||
| 108 | $this->accountManager = $accountManager; |
||
| 109 | $this->logger = $logger; |
||
| 110 | $this->l10nFactory = $l10nFactory; |
||
| 111 | $this->newUserMailHelper = $newUserMailHelper; |
||
| 112 | $this->federatedFileSharingFactory = $federatedFileSharingFactory; |
||
| 113 | } |
||
| 114 | |||
| 115 | /** |
||
| 116 | * @NoAdminRequired |
||
| 117 | * |
||
| 118 | * returns a list of users |
||
| 119 | * |
||
| 120 | * @param string $search |
||
| 121 | * @param int $limit |
||
| 122 | * @param int $offset |
||
| 123 | * @return DataResponse |
||
| 124 | */ |
||
| 125 | public function getUsers($search = '', $limit = null, $offset = null) { |
||
| 126 | $user = $this->userSession->getUser(); |
||
| 127 | $users = []; |
||
| 128 | |||
| 129 | // Admin? Or SubAdmin? |
||
|
|
|||
| 130 | $uid = $user->getUID(); |
||
| 131 | $subAdminManager = $this->groupManager->getSubAdmin(); |
||
| 132 | if($this->groupManager->isAdmin($uid)){ |
||
| 133 | $users = $this->userManager->search($search, $limit, $offset); |
||
| 134 | } else if ($subAdminManager->isSubAdmin($user)) { |
||
| 135 | $subAdminOfGroups = $subAdminManager->getSubAdminsGroups($user); |
||
| 136 | foreach ($subAdminOfGroups as $key => $group) { |
||
| 137 | $subAdminOfGroups[$key] = $group->getGID(); |
||
| 138 | } |
||
| 139 | |||
| 140 | if($offset === null) { |
||
| 141 | $offset = 0; |
||
| 142 | } |
||
| 143 | |||
| 144 | $users = []; |
||
| 145 | foreach ($subAdminOfGroups as $group) { |
||
| 146 | $users = array_merge($users, $this->groupManager->displayNamesInGroup($group, $search)); |
||
| 147 | } |
||
| 148 | |||
| 149 | $users = array_slice($users, $offset, $limit); |
||
| 150 | } |
||
| 151 | |||
| 152 | $users = array_keys($users); |
||
| 153 | |||
| 154 | return new DataResponse([ |
||
| 155 | 'users' => $users |
||
| 156 | ]); |
||
| 157 | } |
||
| 158 | |||
| 159 | /** |
||
| 160 | * @PasswordConfirmationRequired |
||
| 161 | * @NoAdminRequired |
||
| 162 | * |
||
| 163 | * @param string $userid |
||
| 164 | * @param string $password |
||
| 165 | * @param array $groups |
||
| 166 | * @return DataResponse |
||
| 167 | * @throws OCSException |
||
| 168 | */ |
||
| 169 | public function addUser($userid, $password, $groups = null) { |
||
| 170 | $user = $this->userSession->getUser(); |
||
| 171 | $isAdmin = $this->groupManager->isAdmin($user->getUID()); |
||
| 172 | $subAdminManager = $this->groupManager->getSubAdmin(); |
||
| 173 | |||
| 174 | if($this->userManager->userExists($userid)) { |
||
| 175 | $this->logger->error('Failed addUser attempt: User already exists.', ['app' => 'ocs_api']); |
||
| 176 | throw new OCSException('User already exists', 102); |
||
| 177 | } |
||
| 178 | |||
| 179 | if(is_array($groups)) { |
||
| 180 | foreach ($groups as $group) { |
||
| 181 | if(!$this->groupManager->groupExists($group)) { |
||
| 182 | throw new OCSException('group '.$group.' does not exist', 104); |
||
| 183 | } |
||
| 184 | if(!$isAdmin && !$subAdminManager->isSubAdminofGroup($user, $this->groupManager->get($group))) { |
||
| 185 | throw new OCSException('insufficient privileges for group '. $group, 105); |
||
| 186 | } |
||
| 187 | } |
||
| 188 | } else { |
||
| 189 | if(!$isAdmin) { |
||
| 190 | throw new OCSException('no group specified (required for subadmins)', 106); |
||
| 191 | } |
||
| 192 | } |
||
| 193 | |||
| 194 | try { |
||
| 195 | $newUser = $this->userManager->createUser($userid, $password); |
||
| 196 | $this->logger->info('Successful addUser call with userid: '.$userid, ['app' => 'ocs_api']); |
||
| 197 | |||
| 198 | if (is_array($groups)) { |
||
| 199 | foreach ($groups as $group) { |
||
| 200 | $this->groupManager->get($group)->addUser($newUser); |
||
| 201 | $this->logger->info('Added userid '.$userid.' to group '.$group, ['app' => 'ocs_api']); |
||
| 202 | } |
||
| 203 | } |
||
| 204 | return new DataResponse(); |
||
| 205 | } catch (\Exception $e) { |
||
| 206 | $this->logger->logException($e, [ |
||
| 207 | 'message' => 'Failed addUser attempt with exception.', |
||
| 208 | 'level' => \OCP\Util::ERROR, |
||
| 209 | 'app' => 'ocs_api', |
||
| 210 | ]); |
||
| 211 | throw new OCSException('Bad request', 101); |
||
| 212 | } |
||
| 213 | } |
||
| 214 | |||
| 215 | /** |
||
| 216 | * @NoAdminRequired |
||
| 217 | * @NoSubAdminRequired |
||
| 218 | * |
||
| 219 | * gets user info |
||
| 220 | * |
||
| 221 | * @param string $userId |
||
| 222 | * @return DataResponse |
||
| 223 | * @throws OCSException |
||
| 224 | */ |
||
| 225 | public function getUser($userId) { |
||
| 229 | |||
| 230 | /** |
||
| 231 | * @NoAdminRequired |
||
| 232 | * @NoSubAdminRequired |
||
| 233 | * |
||
| 234 | * gets user info from the currently logged in user |
||
| 235 | * |
||
| 236 | * @return DataResponse |
||
| 237 | * @throws OCSException |
||
| 238 | */ |
||
| 239 | public function getCurrentUser() { |
||
| 253 | |||
| 254 | /** |
||
| 255 | * creates a array with all user data |
||
| 256 | * |
||
| 257 | * @param $userId |
||
| 258 | * @return array |
||
| 259 | * @throws OCSException |
||
| 260 | */ |
||
| 261 | protected function getUserData($userId) { |
||
| 304 | |||
| 305 | /** |
||
| 306 | * @NoAdminRequired |
||
| 307 | * @NoSubAdminRequired |
||
| 308 | */ |
||
| 309 | public function getEditableFields() { |
||
| 331 | |||
| 332 | /** |
||
| 333 | * @NoAdminRequired |
||
| 334 | * @NoSubAdminRequired |
||
| 335 | * @PasswordConfirmationRequired |
||
| 336 | * |
||
| 337 | * edit users |
||
| 338 | * |
||
| 339 | * @param string $userId |
||
| 340 | * @param string $key |
||
| 341 | * @param string $value |
||
| 342 | * @return DataResponse |
||
| 343 | * @throws OCSException |
||
| 344 | * @throws OCSForbiddenException |
||
| 345 | */ |
||
| 346 | public function editUser($userId, $key, $value) { |
||
| 468 | |||
| 469 | /** |
||
| 470 | * @PasswordConfirmationRequired |
||
| 471 | * @NoAdminRequired |
||
| 472 | * |
||
| 473 | * @param string $userId |
||
| 474 | * @return DataResponse |
||
| 475 | * @throws OCSException |
||
| 476 | * @throws OCSForbiddenException |
||
| 477 | */ |
||
| 478 | public function deleteUser($userId) { |
||
| 500 | |||
| 501 | /** |
||
| 502 | * @PasswordConfirmationRequired |
||
| 503 | * @NoAdminRequired |
||
| 504 | * |
||
| 505 | * @param string $userId |
||
| 506 | * @return DataResponse |
||
| 507 | * @throws OCSException |
||
| 508 | * @throws OCSForbiddenException |
||
| 509 | */ |
||
| 510 | public function disableUser($userId) { |
||
| 513 | |||
| 514 | /** |
||
| 515 | * @PasswordConfirmationRequired |
||
| 516 | * @NoAdminRequired |
||
| 517 | * |
||
| 518 | * @param string $userId |
||
| 519 | * @return DataResponse |
||
| 520 | * @throws OCSException |
||
| 521 | * @throws OCSForbiddenException |
||
| 522 | */ |
||
| 523 | public function enableUser($userId) { |
||
| 526 | |||
| 527 | /** |
||
| 528 | * @param string $userId |
||
| 529 | * @param bool $value |
||
| 530 | * @return DataResponse |
||
| 531 | * @throws OCSException |
||
| 532 | * @throws OCSForbiddenException |
||
| 533 | */ |
||
| 534 | private function setEnabled($userId, $value) { |
||
| 552 | |||
| 553 | /** |
||
| 554 | * @NoAdminRequired |
||
| 555 | * @NoSubAdminRequired |
||
| 556 | * |
||
| 557 | * @param string $userId |
||
| 558 | * @return DataResponse |
||
| 559 | * @throws OCSException |
||
| 560 | */ |
||
| 561 | public function getUsersGroups($userId) { |
||
| 597 | |||
| 598 | /** |
||
| 599 | * @PasswordConfirmationRequired |
||
| 600 | * @NoAdminRequired |
||
| 601 | * |
||
| 602 | * @param string $userId |
||
| 603 | * @param string $groupid |
||
| 604 | * @return DataResponse |
||
| 605 | * @throws OCSException |
||
| 606 | */ |
||
| 607 | public function addToGroup($userId, $groupid = '') { |
||
| 632 | |||
| 633 | /** |
||
| 634 | * @PasswordConfirmationRequired |
||
| 635 | * @NoAdminRequired |
||
| 636 | * |
||
| 637 | * @param string $userId |
||
| 638 | * @param string $groupid |
||
| 639 | * @return DataResponse |
||
| 640 | * @throws OCSException |
||
| 641 | */ |
||
| 642 | public function removeFromGroup($userId, $groupid) { |
||
| 695 | |||
| 696 | /** |
||
| 697 | * Creates a subadmin |
||
| 698 | * |
||
| 699 | * @PasswordConfirmationRequired |
||
| 700 | * |
||
| 701 | * @param string $userId |
||
| 702 | * @param string $groupid |
||
| 703 | * @return DataResponse |
||
| 704 | * @throws OCSException |
||
| 705 | */ |
||
| 706 | View Code Duplication | public function addSubAdmin($userId, $groupid) { |
|
| 736 | |||
| 737 | /** |
||
| 738 | * Removes a subadmin from a group |
||
| 739 | * |
||
| 740 | * @PasswordConfirmationRequired |
||
| 741 | * |
||
| 742 | * @param string $userId |
||
| 743 | * @param string $groupid |
||
| 744 | * @return DataResponse |
||
| 745 | * @throws OCSException |
||
| 746 | */ |
||
| 747 | View Code Duplication | public function removeSubAdmin($userId, $groupid) { |
|
| 772 | |||
| 773 | /** |
||
| 774 | * Get the groups a user is a subadmin of |
||
| 775 | * |
||
| 776 | * @param string $userId |
||
| 777 | * @return DataResponse |
||
| 778 | * @throws OCSException |
||
| 779 | */ |
||
| 780 | public function getUserSubAdminGroups($userId) { |
||
| 799 | |||
| 800 | /** |
||
| 801 | * @param string $userId |
||
| 802 | * @return array |
||
| 803 | * @throws \OCP\Files\NotFoundException |
||
| 804 | */ |
||
| 805 | protected function fillStorageInfo($userId) { |
||
| 822 | |||
| 823 | /** |
||
| 824 | * @NoAdminRequired |
||
| 825 | * @PasswordConfirmationRequired |
||
| 826 | * |
||
| 827 | * resend welcome message |
||
| 828 | * |
||
| 829 | * @param string $userId |
||
| 830 | * @return DataResponse |
||
| 831 | * @throws OCSException |
||
| 832 | */ |
||
| 833 | public function resendWelcomeMessage($userId) { |
||
| 876 | } |
||
| 877 |
Loading history...
Sometimes obsolete code just ends up commented out instead of removed. In this case it is better to remove the code once you have checked you do not need it.
The code might also have been commented out for debugging purposes. In this case it is vital that someone uncomments it again or your project may behave in very unexpected ways in production.
This check looks for comments that seem to be mostly valid code and reports them.