diff --git a/.gitignore b/.gitignore index 5fc109b5..a6214def 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,4 @@ js .vscode .php_cs.cache .php-cs-fixer.cache +.phpunit.result.cache diff --git a/appinfo/info.xml b/appinfo/info.xml index 68029f90..43bf854d 100755 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -14,7 +14,7 @@ Guests accounts can be created from the share menu by entering either the recipients email or name and choosing "create guest account", once the share is created the guest user will receive an email notification about the mail with a link to set their password. Guests users can only access files shared to them and cannot create any files outside of shares, additionally, the apps accessible to guest accounts are whitelisted.]]> - 4.7.0-dev.0 + 4.7.0-dev.1 agpl Nextcloud diff --git a/lib/Command/AddCommand.php b/lib/Command/AddCommand.php index 703ca116..711d73f6 100644 --- a/lib/Command/AddCommand.php +++ b/lib/Command/AddCommand.php @@ -9,6 +9,7 @@ namespace OCA\Guests\Command; +use OCA\Guests\Config; use OCA\Guests\GuestManager; use OCP\IUser; use OCP\IUserManager; @@ -27,6 +28,7 @@ public function __construct( private readonly IUserManager $userManager, private readonly IMailer $mailer, private readonly GuestManager $guestManager, + private readonly Config $config, ) { parent::__construct(); } @@ -83,14 +85,20 @@ protected function execute(InputInterface $input, OutputInterface $output): int return self::FAILURE; } + $email = $input->getArgument('email'); + if ($this->config->useHashedEmailAsUserID()) { + $email = strtolower($email); + $uid = hash('sha256', $email); + } else { + $uid = $email; + } + // same behavior like in the UsersController - $uid = $input->getArgument('email'); if ($this->userManager->userExists($uid)) { $output->writeln('The user "' . $uid . '" already exists.'); return self::FAILURE; } - $email = $input->getArgument('email'); if (!$this->mailer->validateMailAddress($email)) { $output->writeln('Invalid email address "' . $email . '".'); return self::FAILURE; diff --git a/lib/Command/ListCommand.php b/lib/Command/ListCommand.php index e7c51600..20fa1d1b 100644 --- a/lib/Command/ListCommand.php +++ b/lib/Command/ListCommand.php @@ -49,7 +49,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->writeArrayInOutputFormat($input, $output, $guests); } else { $table = new Table($output); - $table->setHeaders(['Email', 'Name', 'Invited By']); + $table->setHeaders(['Email', 'UserID', 'Name', 'Invited By']); $table->setRows($guests); $table->render(); } diff --git a/lib/Config.php b/lib/Config.php index 83cd8b07..939e747d 100644 --- a/lib/Config.php +++ b/lib/Config.php @@ -33,6 +33,14 @@ public function setAllowExternalStorage(string|bool $allow): void { $this->appConfig->setAppValueBool('allow_external_storage', $allow === true || $allow === 'true') ; } + public function useHashedEmailAsUserID(): bool { + return $this->appConfig->getAppValueBool('hash_user_ids', true); + } + + public function setUseHashedEmailAsUserID(bool $useHash): void { + $this->appConfig->setAppValueBool('hash_user_ids', $useHash) ; + } + public function hideOtherUsers(): bool { return $this->appConfig->getAppValueBool('hide_users', true); } diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 626e0806..c3c4a560 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -48,6 +48,7 @@ public function getConfig(): DataResponse { 'useWhitelist' => $useWhitelist, 'whitelist' => $whitelist, 'allowExternalStorage' => $allowExternalStorage, + 'useHashedEmailAsUserID' => $this->config->useHashedEmailAsUserID(), 'hideUsers' => $hideUsers, 'whiteListableApps' => $this->appWhitelist->getWhitelistAbleApps(), 'sharingRestrictedToGroup' => $this->config->isSharingRestrictedToGroup(), @@ -58,7 +59,7 @@ public function getConfig(): DataResponse { /** * @param list $whitelist */ - public function setConfig(bool $useWhitelist, array $whitelist, bool $allowExternalStorage, bool $hideUsers, array $createRestrictedToGroup): DataResponse { + public function setConfig(bool $useWhitelist, array $whitelist, bool $allowExternalStorage, bool $useHashedEmailAsUserID, bool $hideUsers, array $createRestrictedToGroup): DataResponse { $newWhitelist = []; foreach ($whitelist as $app) { $newWhitelist[] = trim((string)$app); @@ -67,6 +68,7 @@ public function setConfig(bool $useWhitelist, array $whitelist, bool $allowExter $this->config->setUseWhitelist($useWhitelist); $this->config->setAppWhitelist($newWhitelist); $this->config->setAllowExternalStorage($allowExternalStorage); + $this->config->setUseHashedEmailAsUserID($useHashedEmailAsUserID); $this->config->setHideOtherUsers($hideUsers); $this->config->setCreateRestrictedToGroup($createRestrictedToGroup); diff --git a/lib/Controller/UsersController.php b/lib/Controller/UsersController.php index 96c1cf7e..e3a3a0c8 100644 --- a/lib/Controller/UsersController.php +++ b/lib/Controller/UsersController.php @@ -111,7 +111,12 @@ public function create(string $email, string $displayName, string $language, arr ); } - $username = $email; + if ($this->config->useHashedEmailAsUserID()) { + $email = strtolower($email); + $username = hash('sha256', $email); + } else { + $username = $email; + } $existingUsers = $this->userManager->getByEmail($email); if (count($existingUsers) > 0) { diff --git a/lib/GuestManager.php b/lib/GuestManager.php index 0cd2fcfc..c38018a1 100644 --- a/lib/GuestManager.php +++ b/lib/GuestManager.php @@ -69,6 +69,7 @@ public function createGuest(?IUser $createdBy, string $userId, string $email, st return null; } + $this->userBackend->setInitialEmail($userId, $email); $user->setSystemEMailAddress($email); if ($createdBy instanceof IUser) { $this->config->setUserValue($userId, 'guests', 'created_by', $createdBy->getUID()); @@ -125,11 +126,11 @@ public function listGuests(): array { * }> */ public function getGuestsInfo(): array { - $displayNames = $this->userBackend->getDisplayNames(); - $guests = array_keys($displayNames); + $guestsInfo = $this->userBackend->getAllGuestAccounts(); + $guests = array_keys($guestsInfo); $shareCounts = $this->getShareCountForUsers($guests); $createdBy = $this->config->getUserValueForUsers('guests', 'created_by', $guests); - return array_map(function (string $uid) use ($createdBy, $displayNames, $shareCounts): array { + return array_map(function (string $uid) use ($createdBy, $guestsInfo, $shareCounts): array { $allSharesCount = count(array_merge( $this->shareManager->getSharedWith($uid, IShare::TYPE_USER, null, -1, 0), $this->shareManager->getSharedWith($uid, IShare::TYPE_GROUP, null, -1, 0), @@ -138,8 +139,9 @@ public function getGuestsInfo(): array { $this->shareManager->getSharedWith($uid, IShare::TYPE_ROOM, null, -1, 0), )); return [ - 'email' => $uid, - 'display_name' => $displayNames[$uid] ?? $uid, + 'email' => $guestsInfo[$uid]['email'] ?? $uid, + 'uid' => $uid, + 'display_name' => $guestsInfo[$uid]['displayname'] ?? $uid, 'created_by' => $createdBy[$uid] ?? '', 'share_count' => $shareCounts[$uid] ?? 0, 'share_count_with_circles' => $allSharesCount, diff --git a/lib/Migration/Version4002Date20250501195008.php b/lib/Migration/Version4002Date20250501195008.php new file mode 100644 index 00000000..13f355b2 --- /dev/null +++ b/lib/Migration/Version4002Date20250501195008.php @@ -0,0 +1,61 @@ +getTable('guests_users'); + if ($table->hasColumn('email')) { + return null; + } + + $table->addColumn('email', 'string', [ + 'notnull' => false, + 'length' => 64, + 'default' => '', + ]); + return $schema; + } + + /** + * @param IOutput $output + * @param Closure(): ISchemaWrapper $schemaClosure + * @param array $options + */ + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + $query = $this->db->getQueryBuilder(); + $query->update('guests_users') + ->set('email', 'uid_lower'); + $query->executeStatement(); + } +} diff --git a/lib/UserBackend.php b/lib/UserBackend.php index e5e0a930..3e12b439 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -103,6 +103,14 @@ public function deleteUser($uid): bool { return (bool)$result; } + public function setInitialEmail(string $uid, string $email): bool { + $query = $this->dbConn->getQueryBuilder(); + $query->update('guests_users') + ->set('email', $query->createNamedParameter($email)) + ->where($query->expr()->eq('uid_lower', $query->createNamedParameter(mb_strtolower($uid)))); + return (bool)$query->executeStatement(); + } + /** * Change the password of a user */ @@ -231,6 +239,35 @@ public function getDisplayNames($search = '', $limit = null, $offset = null): ar return $displayNames; } + /** + * Get a list of all users with their email and display name + * + * @return array + */ + public function getAllGuestAccounts(?int $limit = null, ?int $offset = null): array { + if (!$this->allowListing) { + return []; + } + + $query = $this->dbConn->getQueryBuilder(); + $query->select('uid', 'email', 'displayname') + ->from('guests_users') + ->orderBy('uid_lower', 'ASC') + ->setMaxResults($limit) + ->setFirstResult($offset); + + $result = $query->executeQuery(); + $users = []; + while ($row = $result->fetch()) { + $users[(string)$row['uid']] = [ + 'email' => (string)$row['email'], + 'displayname' => (string)$row['displayname'], + ]; + } + + return $users; + } + /** * Check if the password is correct without logging in the user * returns the user id or false @@ -238,7 +275,7 @@ public function getDisplayNames($search = '', $limit = null, $offset = null): ar * @return string|false */ public function checkPassword(string $loginName, string $password) { - if (!str_contains($loginName, '@')) { + if (!$this->potentialGuestUserId($loginName)) { return false; } @@ -275,9 +312,13 @@ public function checkPassword(string $loginName, string $password) { * @param string $uid the username */ private function loadUser($uid): bool { + if (isset($this->cache[$uid]) && $this->cache[$uid] === false) { + return false; + } + // guests $uid could be NULL or '' // or is not an email anyway - if (!str_contains($uid, '@')) { + if (!$this->potentialGuestUserId($uid)) { $this->cache[$uid] = false; return false; } @@ -390,7 +431,7 @@ public function getBackendName(): string { } public function getRealUID(string $uid): string { - if (!str_contains($uid, '@')) { + if (!$this->potentialGuestUserId($uid)) { throw new \RuntimeException($uid . ' does not exist'); } @@ -404,4 +445,16 @@ public function getRealUID(string $uid): string { return $this->cache[$uid]['uid']; } + + /** + * Guest app user ids are: + * - either email addresses so they need to contain an @ + * - lowercase sha256 hashes of email addresses, 64 characters of a-f and 0-9 + * + * @param string $userId + * @return bool + */ + protected function potentialGuestUserId(string $userId): bool { + return str_contains($userId, '@') || preg_match('/^[a-f0-9]{64}$/', $userId); + } } diff --git a/src/components/GuestList.vue b/src/components/GuestList.vue index 4615964f..a712337d 100644 --- a/src/components/GuestList.vue +++ b/src/components/GuestList.vue @@ -11,6 +11,9 @@ {{ t('guests', 'Email') }} + + {{ t('guests', 'User ID') }} + {{ t('guests', 'Name') }} @@ -31,6 +34,10 @@ :title="guest.email"> {{ guest.email }} + + {{ guest.uid }} + {{ guest.display_name }} @@ -46,7 +53,7 @@ - + diff --git a/src/views/GuestSettings.vue b/src/views/GuestSettings.vue index 0fe6045c..3e90301f 100644 --- a/src/views/GuestSettings.vue +++ b/src/views/GuestSettings.vue @@ -39,6 +39,12 @@ {{ t('guests', 'Guest accounts can access mounted external storages') }} + + {{ t('guests', 'Use a hash of the email as user ID for improved privacy') }} + + @@ -133,6 +139,7 @@ export default { config: { useWhitelist: false, allowExternalStorage: false, + useHashedEmailAsUserID: true, hideUsers: false, whitelist: [], whiteListableApps: [], diff --git a/tests/unit/Command/AddCommandTest.php b/tests/unit/Command/AddCommandTest.php index c3fad58d..ca950afb 100644 --- a/tests/unit/Command/AddCommandTest.php +++ b/tests/unit/Command/AddCommandTest.php @@ -9,6 +9,7 @@ namespace OCA\Guests\Test\Command\Unit; use OCA\Guests\Command\AddCommand; +use OCA\Guests\Config; use OCA\Guests\GuestManager; use OCP\IUser; use OCP\IUserManager; @@ -19,14 +20,10 @@ use Test\TestCase; class AddCommandTest extends TestCase { - /** @var IUserManager|MockObject */ - private $userManager; - - /** @var GuestManager|MockObject */ - private $guestManager; - - /** @var IMailer|MockObject */ - private $mailer; + private IUserManager&MockObject $userManager; + private GuestManager&MockObject $guestManager; + private IMailer&MockObject $mailer; + private Config&MockObject $config; private ?AddCommand $command = null; @@ -38,11 +35,13 @@ protected function setUp(): void { $this->userManager = $this->createMock(IUserManager::class); $this->guestManager = $this->createMock(GuestManager::class); $this->mailer = $this->createMock(IMailer::class); + $this->config = $this->createMock(Config::class); $this->command = new AddCommand( $this->userManager, $this->mailer, - $this->guestManager + $this->guestManager, + $this->config, ); $this->command->setApplication(new Application()); diff --git a/tests/unit/Migration/OwncloudGuestMigrationTest.php b/tests/unit/Migration/OwncloudGuestMigrationTest.php index 9e742cc1..0b872b78 100644 --- a/tests/unit/Migration/OwncloudGuestMigrationTest.php +++ b/tests/unit/Migration/OwncloudGuestMigrationTest.php @@ -45,6 +45,7 @@ protected function setUp():void { } private function createOcGuest(string $userId): void { + $this->userManager->get($userId)?->delete(); $this->userManager->createUser($userId, $userId . '_password'); $this->config->setUserValue($userId, 'owncloud', 'isGuest', '1'); }