Merge pull request #13138 from nextcloud/enhancement/noid/ldap-remnants-detected-field

register and show when an LDAP user was detected as unavailable/deleted

apps/user_ldap/lib/Command/ShowRemnants.php

@@ -55,38 +55,49 @@ class ShowRemnants extends Command {
 		$this
 			->setName('ldap:show-remnants')
 			->setDescription('shows which users are not available on LDAP anymore, but have remnants in Nextcloud.')
-			->addOption('json', null, InputOption::VALUE_NONE, 'return JSON array instead of pretty table.');
+			->addOption('json', null, InputOption::VALUE_NONE, 'return JSON array instead of pretty table.')
+			->addOption('short-date', null, InputOption::VALUE_NONE, 'show dates in Y-m-d format');
+	}
+
+	protected function formatDate(int $timestamp, string $default, bool $showShortDate) {
+		if (!($timestamp > 0)) {
+			return $default;
+		}
+		if ($showShortDate) {
+			return date('Y-m-d', $timestamp);
+		}
+		return $this->dateFormatter->formatDate($timestamp);
 	}
 
 	/**
-	 * executes the command, i.e. creeates and outputs a table of LDAP users marked as deleted
+	 * executes the command, i.e. creates and outputs a table of LDAP users marked as deleted
 	 *
 	 * {@inheritdoc}
 	 */
 	protected function execute(InputInterface $input, OutputInterface $output) {
 		/** @var \Symfony\Component\Console\Helper\Table $table */
 		$table = new Table($output);
-		$table->setHeaders(array(
+		$table->setHeaders([
 			'Nextcloud name', 'Display Name', 'LDAP UID', 'LDAP DN', 'Last Login',
-			'Dir', 'Sharer'));
+			'Detected on', 'Dir', 'Sharer'
-		$rows = array();
+		]);
+		$rows = [];
 		$resultSet = $this->dui->getUsers();
-		foreach($resultSet as $user) {
+		foreach ($resultSet as $user) {
-			$hAS = $user->getHasActiveShares() ? 'Y' : 'N';
+			$rows[] = [
-			$lastLogin = ($user->getLastLogin() > 0) ?
+				'ocName' => $user->getOCName(),
-				$this->dateFormatter->formatDate($user->getLastLogin()) : '-';
+				'displayName' => $user->getDisplayName(),
-			$rows[] = array('ocName'      => $user->getOCName(),
+				'uid' => $user->getUID(),
-							'displayName' => $user->getDisplayName(),
+				'dn' => $user->getDN(),
-							'uid'         => $user->getUID(),
+				'lastLogin' => $this->formatDate($user->getLastLogin(), '-', (bool)$input->getOption('short-date')),
-							'dn'          => $user->getDN(),
+				'detectedOn' => $this->formatDate($user->getDetectedOn(), 'unknown', (bool)$input->getOption('short-date')),
-							'lastLogin'   => $lastLogin,
+				'homePath' => $user->getHomePath(),
-							'homePath'    => $user->getHomePath(),
+				'sharer' => $user->getHasActiveShares() ? 'Y' : 'N',
-							'sharer'      => $hAS
+			];
-			);
 		}
 
 		if ($input->getOption('json')) {
-			$output->writeln(json_encode($rows));			
+			$output->writeln(json_encode($rows));
 		} else {
 			$table->setRows($rows);
 			$table->render($output);

apps/user_ldap/lib/User/DeletedUsersIndex.php

@@ -70,7 +70,7 @@ class DeletedUsersIndex {
 		$deletedUsers = $this->config->getUsersForUserValue(
 			'user_ldap', 'isDeleted', '1');
 
-		$userObjects = array();
+		$userObjects = [];
 		foreach($deletedUsers as $user) {
 			$userObjects[] = new OfflineUser($user, $this->config, $this->db, $this->mapping);
 		}
@@ -95,20 +95,26 @@ class DeletedUsersIndex {
 	 * @return bool
 	 */
 	public function hasUsers() {
-		if($this->deletedUsers === false) {
+		if(!is_array($this->deletedUsers)) {
 			$this->fetchDeletedUsers();
 		}
-		if(is_array($this->deletedUsers) && count($this->deletedUsers) > 0) {
+		return is_array($this->deletedUsers) && (count($this->deletedUsers) > 0);
-			return true;
-		}
-		return false;
 	}
 
 	/**
 	 * marks a user as deleted
+	 *
 	 * @param string $ocName
+	 * @throws \OCP\PreConditionNotMetException
 	 */
 	public function markUser($ocName) {
+		$curValue = $this->config->getUserValue($ocName, 'user_ldap', 'isDeleted', '0');
+		if($curValue === '1') {
+			// the user is already marked, do not write to DB again
+			return;
+		}
 		$this->config->setUserValue($ocName, 'user_ldap', 'isDeleted', '1');
+		$this->config->setUserValue($ocName, 'user_ldap', 'foundDeleted', (string)time());
+		$this->deletedUsers = null;
 	}
 }

apps/user_ldap/lib/User/OfflineUser.php

@@ -53,6 +53,10 @@ class OfflineUser {
 	 * @var string $lastLogin the timestamp of the last login
 	 */
 	protected $lastLogin;
+	/**
+	 * @var string $foundDeleted the timestamp when the user was detected as unavailable
+	 */
+	protected $foundDeleted;
 	/**
 	 * @var string $email
 	 */
@@ -92,7 +96,8 @@ class OfflineUser {
 	 * remove the Delete-flag from the user.
 	 */
 	public function unmark() {
-		$this->config->setUserValue($this->ocName, 'user_ldap', 'isDeleted', '0');
+		$this->config->deleteUserValue($this->ocName, 'user_ldap', 'isDeleted');
+		$this->config->deleteUserValue($this->ocName, 'user_ldap', 'foundDeleted');
 	}
 
 	/**
@@ -169,6 +174,14 @@ class OfflineUser {
 		return (int)$this->lastLogin;
 	}
 
+	/**
+	 * getter for the detection timestamp
+	 * @return int
+	 */
+	public function getDetectedOn() {
+		return (int)$this->foundDeleted;
+	}
+
 	/**
 	 * getter for having active shares
 	 * @return bool
@@ -181,13 +194,14 @@ class OfflineUser {
 	 * reads the user details
 	 */
 	protected function fetchDetails() {
-		$properties = array (
+		$properties = [
-			'displayName' => 'user_ldap',
+			'displayName'  => 'user_ldap',
-			'uid'         => 'user_ldap',
+			'uid'          => 'user_ldap',
-			'homePath'    => 'user_ldap',
+			'homePath'     => 'user_ldap',
-			'email'       => 'settings',
+			'foundDeleted' => 'user_ldap',
-			'lastLogin'   => 'login'
+			'email'        => 'settings',
-		);
+			'lastLogin'    => 'login',
+		];
 		foreach($properties as $property => $app) {
 			$this->$property = $this->config->getUserValue($this->ocName, $app, $property, '');
 		}

apps/user_ldap/tests/User/DeletedUsersIndexTest.php

@@ -0,0 +1,121 @@
+<?php
+/**
+ * @copyright Copyright (c) 2018 Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCA\User_LDAP\Tests\User;
+
+use OCA\User_LDAP\Mapping\UserMapping;
+use OCA\User_LDAP\User\DeletedUsersIndex;
+use OCP\IConfig;
+use OCP\IDBConnection;
+
+/**
+ * Class DeletedUsersIndexTest
+ *
+ * @group DB
+ *
+ * @package OCA\User_LDAP\Tests\User
+ */
+class DeletedUsersIndexTest extends \Test\TestCase {
+	/** @var DeletedUsersIndex */
+	protected $dui;
+
+	/** @var IConfig */
+	protected $config;
+
+	/** @var IDBConnection */
+	protected $db;
+
+	/** @var UserMapping|\PHPUnit_Framework_MockObject_MockObject */
+	protected $mapping;
+
+	public function setUp() {
+		parent::setUp();
+
+		// no mocks for those as tests go against DB
+		$this->config = \OC::$server->getConfig();
+		$this->db = \OC::$server->getDatabaseConnection();
+
+		// ensure a clean database
+		$this->config->deleteAppFromAllUsers('user_ldap');
+
+		$this->mapping = $this->createMock(UserMapping::class);
+
+		$this->dui = new DeletedUsersIndex($this->config, $this->db, $this->mapping);
+	}
+
+	public function tearDown() {
+		$this->config->deleteAppFromAllUsers('user_ldap');
+		return parent::tearDown();
+	}
+
+	public function testMarkAndFetchUser() {
+		$uids = [
+			'cef3775c-71d2-48eb-8984-39a4051b0b95',
+			'8c4bbb40-33ed-42d0-9b14-85b0ab76c1cc',
+		];
+
+		// ensure test works on a pristine state
+		$this->assertFalse($this->dui->hasUsers());
+
+		$this->dui->markUser($uids[0]);
+
+		$this->assertTrue($this->dui->hasUsers());
+
+		$this->dui->markUser($uids[1]);
+
+		$deletedUsers = $this->dui->getUsers();
+		$this->assertSame(2, count($deletedUsers));
+
+		// ensure the different uids were used
+		foreach($deletedUsers as $deletedUser) {
+			$this->assertTrue(in_array($deletedUser->getOCName(), $uids));
+			$i = array_search($deletedUser->getOCName(), $uids);
+			$this->assertNotFalse($i);
+			unset($uids[$i]);
+		}
+		$this->assertEmpty($uids);
+	}
+
+	public function testUnmarkUser() {
+		$uids = [
+			'22a162c7-a9ee-487c-9f33-0563795583fb',
+			'1fb4e0da-4a75-47f3-8fa7-becc7e35c9c5',
+		];
+
+		// we know this works, because of "testMarkAndFetchUser"
+		$this->dui->markUser($uids[0]);
+		// this returns a working instance of OfflineUser
+		$testUser = $this->dui->getUsers()[0];
+		$testUser->unmark();
+
+		// the DUI caches the users, to clear mark someone else
+		$this->dui->markUser($uids[1]);
+
+		$deletedUsers = $this->dui->getUsers();
+		foreach ($deletedUsers as $deletedUser) {
+			$this->assertNotSame($testUser->getOCName(), $deletedUser->getOCName());
+		}
+	}
+
+
+}