Move user creation to a separate service

Move user creation to a separate service. Signed-off-by: Sujith H <[email protected]>
owncloud · Jan 7, 2020 · 0dd2a2e · 0dd2a2e
1 parent 7efdf97
commit 0dd2a2e
Show file tree

Hide file tree

Showing 32 changed files with 2,310 additions and 103 deletions.
diff --git a/apps/provisioning_api/appinfo/routes.php b/apps/provisioning_api/appinfo/routes.php
@@ -27,6 +27,9 @@
 
 namespace OCA\Provisioning_API\AppInfo;
 
+use OC\User\Service\CreateUserService;
+use OC\User\Service\PasswordGeneratorService;
+use OC\User\Service\UserSendMailService;
 use OCA\Provisioning_API\Apps;
 use OCA\Provisioning_API\Groups;
 use OCA\Provisioning_API\Users;
@@ -37,6 +40,20 @@
 	\OC::$server->getUserManager(),
 	\OC::$server->getGroupManager(),
 	\OC::$server->getUserSession(),
+	new CreateUserService(
+		\OC::$server->getUserSession(), \OC::$server->getGroupManager(),
+		\OC::$server->getUserManager(), \OC::$server->getSecureRandom(),
+		\OC::$server->getLogger(),
+		new UserSendMailService(
+			\OC::$server->getSecureRandom(), \OC::$server->getConfig(),
+			\OC::$server->getMailer(), \OC::$server->getURLGenerator(),
+			new \OC_Defaults(), \OC::$server->getTimeFactory(),
+			\OC::$server->getL10N('settings')
+		),
+		new PasswordGeneratorService(
+			\OC::$server->getEventDispatcher(), \OC::$server->getSecureRandom()
+		)
+	),
 	\OC::$server->getLogger(),
 	\OC::$server->getTwoFactorAuthManager()
 );

diff --git a/apps/provisioning_api/lib/Users.php b/apps/provisioning_api/lib/Users.php
@@ -30,6 +30,7 @@
 namespace OCA\Provisioning_API;
 
 use OC\OCS\Result;
+use OC\User\Service\CreateUserService;
 use OC_Helper;
 use OCP\API;
 use OCP\Files\FileInfo;
@@ -50,25 +51,33 @@ class Users {
 	private $groupManager;
 	/** @var IUserSession */
 	private $userSession;
+	/** @var CreateUserService */
+	private $createUserService;
 	/** @var ILogger */
 	private $logger;
 	/** @var \OC\Authentication\TwoFactorAuth\Manager */
 	private $twoFactorAuthManager;
 
 	/**
+	 * Users constructor.
+	 *
 	 * @param IUserManager $userManager
 	 * @param IGroupManager $groupManager
 	 * @param IUserSession $userSession
+	 * @param CreateUserService $createUserService
 	 * @param ILogger $logger
+	 * @param \OC\Authentication\TwoFactorAuth\Manager $twoFactorAuthManager
 	 */
 	public function __construct(IUserManager $userManager,
 								IGroupManager $groupManager,
 								IUserSession $userSession,
+								CreateUserService $createUserService,
 								ILogger $logger,
 								\OC\Authentication\TwoFactorAuth\Manager $twoFactorAuthManager) {
 		$this->userManager = $userManager;
 		$this->groupManager = $groupManager;
 		$this->userSession = $userSession;
+		$this->createUserService = $createUserService;
 		$this->logger = $logger;
 		$this->twoFactorAuthManager = $twoFactorAuthManager;
 	}
@@ -125,8 +134,9 @@ public function getUsers() {
 	 */
 	public function addUser() {
 		$userId = isset($_POST['userid']) ? $_POST['userid'] : null;
-		$password = isset($_POST['password']) ? $_POST['password'] : null;
-		$groups = isset($_POST['groups']) ? $_POST['groups'] : null;
+		$password = isset($_POST['password']) ? $_POST['password'] : '';
+		$groups = isset($_POST['groups']) ? $_POST['groups'] : [];
+		$emailAddress = isset($_POST['email']) ? $_POST['email'] : '';
 		$user = $this->userSession->getUser();
 		$isAdmin = $this->groupManager->isAdmin($user->getUID());
 		$subAdminManager = $this->groupManager->getSubAdmin();
@@ -140,7 +150,7 @@ public function addUser() {
 			return new Result(null, 102, 'User already exists');
 		}
 
-		if (\is_array($groups)) {
+		if (\is_array($groups) && (\count($groups) > 0)) {
 			foreach ($groups as $group) {
 				if (!$this->groupManager->groupExists($group)) {
 					return new Result(null, 104, 'group '.$group.' does not exist');
@@ -156,13 +166,14 @@ public function addUser() {
 		}
 
 		try {
-			$newUser = $this->userManager->createUser($userId, $password);
+			$newUser = $this->createUserService->createUser(['username' => $userId, 'password' => $password, 'email' => $emailAddress]);
 			$this->logger->info('Successful addUser call with userid: '.$userId, ['app' => 'ocs_api']);
 
-			if (\is_array($groups)) {
-				foreach ($groups as $group) {
-					$this->groupManager->get($group)->addUser($newUser);
-					$this->logger->info('Added userid '.$userId.' to group '.$group, ['app' => 'ocs_api']);
+			if (\count($groups) > 0) {
+				$failedToAddGroups = $this->createUserService->addUserToGroups($newUser, $groups);
+				if (\count($failedToAddGroups) > 0) {
+					$failedToAddGroups = \implode(',', $failedToAddGroups);
+					$this->logger->error("User $userId could not be added to groups $failedToAddGroups");
 				}
 			}
 			return new Result(null, 100);

diff --git a/apps/provisioning_api/tests/UsersTest.php b/apps/provisioning_api/tests/UsersTest.php
@@ -30,6 +30,7 @@
 namespace OCA\Provisioning_API\Tests;
 
 use OC\OCS\Result;
+use OC\User\Service\CreateUserService;
 use OCA\Provisioning_API\Users;
 use OCP\API;
 use OCP\ILogger;
@@ -56,6 +57,8 @@ class UsersTest extends OriginalTest {
 	protected $api;
 	/** @var \OC\Authentication\TwoFactorAuth\Manager | PHPUnit\Framework\MockObject\MockObject */
 	private $twoFactorAuthManager;
+	/** @var CreateUserService | PHPUnit\Framework\MockObject\MockObject */
+	private $createUserService;
 
 	protected function tearDown(): void {
 		$_GET = null;
@@ -79,11 +82,13 @@ protected function setUp(): void {
 		$this->twoFactorAuthManager->expects($this->any())
 			->method('isTwoFactorAuthenticated')
 			->willReturn(false);
+		$this->createUserService = $this->createMock(CreateUserService::class);
 		$this->api = $this->getMockBuilder(Users::class)
 			->setConstructorArgs([
 				$this->userManager,
 				$this->groupManager,
 				$this->userSession,
+				$this->createUserService,
 				$this->logger,
 				$this->twoFactorAuthManager
 			])
@@ -324,15 +329,17 @@ public function testAddUserExistingGroupNonExistingGroup() {
 	public function testAddUserSuccessful() {
 		$_POST['userid'] = 'NewUser';
 		$_POST['password'] = 'PasswordOfTheNewUser';
+		$newUserCreated = $this->createMock(IUser::class);
 		$this->userManager
 			->expects($this->once())
 			->method('userExists')
 			->with('NewUser')
 			->will($this->returnValue(false));
-		$this->userManager
+		$this->createUserService
 			->expects($this->once())
 			->method('createUser')
-			->with('NewUser', 'PasswordOfTheNewUser');
+			->with(['username' => 'NewUser', 'password' => 'PasswordOfTheNewUser', 'email' => ''])
+			->will($this->returnValue($newUserCreated));
 		$this->logger
 			->expects($this->once())
 			->method('info')
@@ -385,27 +392,22 @@ public function testAddUserExistingGroup() {
 			->with('ExistingGroup')
 			->willReturn(true);
 		$user = $this->createMock(IUser::class);
-		$this->userManager
+		$this->createUserService
 			->expects($this->once())
 			->method('createUser')
-			->with('NewUser', 'PasswordOfTheNewUser')
+			->with(['username' => 'NewUser', 'password' => 'PasswordOfTheNewUser', 'email' => ''])
 			->willReturn($user);
-		$group = $this->createMock(IGroup::class);
-		$group
-			->expects($this->once())
-			->method('addUser')
-			->with($user);
-		$this->groupManager
+		$this->createUserService
 			->expects($this->once())
-			->method('get')
-			->with('ExistingGroup')
-			->willReturn($group);
+			->method('addUserToGroups')
+			->with($user, ['ExistingGroup'])
+			->will($this->returnValue([]));
+		$group = $this->createMock(IGroup::class);
 		$this->logger
-			->expects($this->exactly(2))
+			->expects($this->exactly(1))
 			->method('info')
 			->withConsecutive(
-				['Successful addUser call with userid: NewUser', ['app' => 'ocs_api']],
-				['Added userid NewUser to group ExistingGroup', ['app' => 'ocs_api']]
+				['Successful addUser call with userid: NewUser', ['app' => 'ocs_api']]
 			);
 
 		$expected = new Result(null, 100);
@@ -420,10 +422,10 @@ public function testAddUserUnsuccessful() {
 			->method('userExists')
 			->with('NewUser')
 			->will($this->returnValue(false));
-		$this->userManager
+		$this->createUserService
 			->expects($this->once())
 			->method('createUser')
-			->with('NewUser', 'PasswordOfTheNewUser')
+			->with(['username' => 'NewUser', 'password' => 'PasswordOfTheNewUser', 'email' => ''])
 			->will($this->throwException(new \Exception('User backend not found.')));
 		$this->logger
 			->expects($this->once())
@@ -599,27 +601,22 @@ public function testAddUserAsSubAdminExistingGroups() {
 			)
 			->willReturn(true);
 		$user = $this->createMock(IUser::class);
-		$this->userManager
+		$this->createUserService
 			->expects($this->once())
 			->method('createUser')
-			->with('NewUser', 'PasswordOfTheNewUser')
+			->with(['username' => 'NewUser', 'password' => 'PasswordOfTheNewUser', 'email' => ''])
 			->willReturn($user);
+		$this->createUserService
+			->expects($this->once())
+			->method('addUserToGroups')
+			->with($user, ['ExistingGroup1', 'ExistingGroup2'])
+			->willReturn([]);
 		$existingGroup1 = $this->createMock(IGroup::class);
 		$existingGroup2 = $this->createMock(IGroup::class);
-		$existingGroup1
-			->expects($this->once())
-			->method('addUser')
-			->with($user);
-		$existingGroup2
-			->expects($this->once())
-			->method('addUser')
-			->with($user);
 		$this->groupManager
-			->expects($this->exactly(4))
+			->expects($this->exactly(2))
 			->method('get')
 			->withConsecutive(
-				['ExistingGroup1'],
-				['ExistingGroup2'],
 				['ExistingGroup1'],
 				['ExistingGroup2']
 			)
@@ -628,12 +625,10 @@ public function testAddUserAsSubAdminExistingGroups() {
 				['ExistingGroup2', $existingGroup2]
 			]));
 		$this->logger
-			->expects($this->exactly(3))
+			->expects($this->exactly(1))
 			->method('info')
 			->withConsecutive(
-				['Successful addUser call with userid: NewUser', ['app' => 'ocs_api']],
-				['Added userid NewUser to group ExistingGroup1', ['app' => 'ocs_api']],
-				['Added userid NewUser to group ExistingGroup2', ['app' => 'ocs_api']]
+				['Successful addUser call with userid: NewUser', ['app' => 'ocs_api']]
 			);
 		$subAdminManager = $this->getMockBuilder(SubAdmin::class)
 			->disableOriginalConstructor()->getMock();

diff --git a/changelog/unreleased/36145 b/changelog/unreleased/36145
@@ -0,0 +1,11 @@
+Change: Move user creation to a separate service
+
+The user creation logic is moved to a separate service.
+Instead of using user managers createUser directly, developers
+can now use methods from CreateUserService.
+Another idea behind this change is to make UI and CLI behave
+same way when user is created. This change would help users
+to be created using the email address. The provisioning api
+is also updated to behave the same way.
+
+https://github.com/owncloud/core/pull/36145
diff --git a/core/Application.php b/core/Application.php
@@ -48,6 +48,7 @@
 use OCP\IConfig;
 use OCP\ILogger;
 use OCP\IServerContainer;
+use OCP\IURLGenerator;
 use OCP\Util;
 
 /**
@@ -91,7 +92,11 @@ public function __construct(array $urlParams= []) {
 				$c->query('AppName'),
 				$c->query('Request'),
 				$c->query('UserManager'),
-				$c->query('Defaults')
+				$c->query('Defaults'),
+				$c->query('OC\User\Service\UserSendMailService'),
+				$c->query('URLGenerator'),
+				$c->query('Logger'),
+				$c->query('L10N')
 			);
 		});
 		$container->registerService('AvatarController', static function (SimpleContainer $c) {