Skip to content

Commit 555cf1b

Browse files
committed
feat: Hash user IDs by default for improved privacy
Signed-off-by: Joas Schilling <coding@schilljs.com>
1 parent e1b4cff commit 555cf1b

14 files changed

Lines changed: 178 additions & 24 deletions

File tree

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,4 @@ js
1111
.vscode
1212
.php_cs.cache
1313
.php-cs-fixer.cache
14+
.phpunit.result.cache

appinfo/info.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
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.
1515
1616
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.]]></description>
17-
<version>4.7.0-dev.0</version>
17+
<version>4.7.0-dev.1</version>
1818
<licence>agpl</licence>
1919
<author>Nextcloud</author>
2020
<types>

lib/Command/AddCommand.php

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
namespace OCA\Guests\Command;
1111

12+
use OCA\Guests\Config;
1213
use OCA\Guests\GuestManager;
1314
use OCP\IUser;
1415
use OCP\IUserManager;
@@ -27,6 +28,7 @@ public function __construct(
2728
private readonly IUserManager $userManager,
2829
private readonly IMailer $mailer,
2930
private readonly GuestManager $guestManager,
31+
private readonly Config $config,
3032
) {
3133
parent::__construct();
3234
}
@@ -83,14 +85,20 @@ protected function execute(InputInterface $input, OutputInterface $output): int
8385
return self::FAILURE;
8486
}
8587

88+
$email = $input->getArgument('email');
89+
if ($this->config->useHashedEmailAsUserID()) {
90+
$email = strtolower($email);
91+
$uid = hash('sha256', $email);
92+
} else {
93+
$uid = $email;
94+
}
95+
8696
// same behavior like in the UsersController
87-
$uid = $input->getArgument('email');
8897
if ($this->userManager->userExists($uid)) {
8998
$output->writeln('<error>The user "' . $uid . '" already exists.</error>');
9099
return self::FAILURE;
91100
}
92101

93-
$email = $input->getArgument('email');
94102
if (!$this->mailer->validateMailAddress($email)) {
95103
$output->writeln('<error>Invalid email address "' . $email . '".</error>');
96104
return self::FAILURE;

lib/Command/ListCommand.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
4949
$this->writeArrayInOutputFormat($input, $output, $guests);
5050
} else {
5151
$table = new Table($output);
52-
$table->setHeaders(['Email', 'Name', 'Invited By']);
52+
$table->setHeaders(['Email', 'UserID', 'Name', 'Invited By']);
5353
$table->setRows($guests);
5454
$table->render();
5555
}

lib/Config.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,14 @@ public function setAllowExternalStorage(string|bool $allow): void {
3333
$this->appConfig->setAppValueBool('allow_external_storage', $allow === true || $allow === 'true') ;
3434
}
3535

36+
public function useHashedEmailAsUserID(): bool {
37+
return $this->appConfig->getAppValueBool('hash_user_ids', true);
38+
}
39+
40+
public function setUseHashedEmailAsUserID(bool $useHash): void {
41+
$this->appConfig->setAppValueBool('hash_user_ids', $useHash) ;
42+
}
43+
3644
public function hideOtherUsers(): bool {
3745
return $this->appConfig->getAppValueBool('hide_users', true);
3846
}

lib/Controller/SettingsController.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ public function getConfig(): DataResponse {
4848
'useWhitelist' => $useWhitelist,
4949
'whitelist' => $whitelist,
5050
'allowExternalStorage' => $allowExternalStorage,
51+
'useHashedEmailAsUserID' => $this->config->useHashedEmailAsUserID(),
5152
'hideUsers' => $hideUsers,
5253
'whiteListableApps' => $this->appWhitelist->getWhitelistAbleApps(),
5354
'sharingRestrictedToGroup' => $this->config->isSharingRestrictedToGroup(),
@@ -58,7 +59,7 @@ public function getConfig(): DataResponse {
5859
/**
5960
* @param list<string> $whitelist
6061
*/
61-
public function setConfig(bool $useWhitelist, array $whitelist, bool $allowExternalStorage, bool $hideUsers, array $createRestrictedToGroup): DataResponse {
62+
public function setConfig(bool $useWhitelist, array $whitelist, bool $allowExternalStorage, bool $useHashedEmailAsUserID, bool $hideUsers, array $createRestrictedToGroup): DataResponse {
6263
$newWhitelist = [];
6364
foreach ($whitelist as $app) {
6465
$newWhitelist[] = trim((string)$app);
@@ -67,6 +68,7 @@ public function setConfig(bool $useWhitelist, array $whitelist, bool $allowExter
6768
$this->config->setUseWhitelist($useWhitelist);
6869
$this->config->setAppWhitelist($newWhitelist);
6970
$this->config->setAllowExternalStorage($allowExternalStorage);
71+
$this->config->setUseHashedEmailAsUserID($useHashedEmailAsUserID);
7072
$this->config->setHideOtherUsers($hideUsers);
7173
$this->config->setCreateRestrictedToGroup($createRestrictedToGroup);
7274

lib/Controller/UsersController.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,12 @@ public function create(string $email, string $displayName, string $language, arr
111111
);
112112
}
113113

114-
$username = $email;
114+
if ($this->config->useHashedEmailAsUserID()) {
115+
$email = strtolower($email);
116+
$username = hash('sha256', $email);
117+
} else {
118+
$username = $email;
119+
}
115120

116121
$existingUsers = $this->userManager->getByEmail($email);
117122
if (count($existingUsers) > 0) {

lib/GuestManager.php

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ public function createGuest(?IUser $createdBy, string $userId, string $email, st
6969
return null;
7070
}
7171

72+
$this->userBackend->setInitialEmail($userId, $email);
7273
$user->setSystemEMailAddress($email);
7374
if ($createdBy instanceof IUser) {
7475
$this->config->setUserValue($userId, 'guests', 'created_by', $createdBy->getUID());
@@ -125,11 +126,11 @@ public function listGuests(): array {
125126
* }>
126127
*/
127128
public function getGuestsInfo(): array {
128-
$displayNames = $this->userBackend->getDisplayNames();
129-
$guests = array_keys($displayNames);
129+
$guestsInfo = $this->userBackend->getAllGuestAccounts();
130+
$guests = array_keys($guestsInfo);
130131
$shareCounts = $this->getShareCountForUsers($guests);
131132
$createdBy = $this->config->getUserValueForUsers('guests', 'created_by', $guests);
132-
return array_map(function (string $uid) use ($createdBy, $displayNames, $shareCounts): array {
133+
return array_map(function (string $uid) use ($createdBy, $guestsInfo, $shareCounts): array {
133134
$allSharesCount = count(array_merge(
134135
$this->shareManager->getSharedWith($uid, IShare::TYPE_USER, null, -1, 0),
135136
$this->shareManager->getSharedWith($uid, IShare::TYPE_GROUP, null, -1, 0),
@@ -138,8 +139,9 @@ public function getGuestsInfo(): array {
138139
$this->shareManager->getSharedWith($uid, IShare::TYPE_ROOM, null, -1, 0),
139140
));
140141
return [
141-
'email' => $uid,
142-
'display_name' => $displayNames[$uid] ?? $uid,
142+
'email' => $guestsInfo[$uid]['email'] ?? $uid,
143+
'uid' => $uid,
144+
'display_name' => $guestsInfo[$uid]['displayname'] ?? $uid,
143145
'created_by' => $createdBy[$uid] ?? '',
144146
'share_count' => $shareCounts[$uid] ?? 0,
145147
'share_count_with_circles' => $allSharesCount,
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Guests\Migration;
11+
12+
use Closure;
13+
use OCP\DB\ISchemaWrapper;
14+
use OCP\IDBConnection;
15+
use OCP\Migration\IOutput;
16+
use OCP\Migration\SimpleMigrationStep;
17+
18+
/**
19+
* Add a column for the email so we know the originally invited email as well
20+
*/
21+
class Version4002Date20250501195008 extends SimpleMigrationStep {
22+
public function __construct(
23+
protected IDBConnection $db,
24+
) {
25+
}
26+
27+
/**
28+
* @param IOutput $output
29+
* @param Closure(): ISchemaWrapper $schemaClosure
30+
* @param array $options
31+
* @return null|ISchemaWrapper
32+
*/
33+
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
34+
/** @var ISchemaWrapper $schema */
35+
$schema = $schemaClosure();
36+
37+
$table = $schema->getTable('guests_users');
38+
if ($table->hasColumn('email')) {
39+
return null;
40+
}
41+
42+
$table->addColumn('email', 'string', [
43+
'notnull' => false,
44+
'length' => 64,
45+
'default' => '',
46+
]);
47+
return $schema;
48+
}
49+
50+
/**
51+
* @param IOutput $output
52+
* @param Closure(): ISchemaWrapper $schemaClosure
53+
* @param array $options
54+
*/
55+
public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void {
56+
$query = $this->db->getQueryBuilder();
57+
$query->update('guests_users')
58+
->set('email', 'uid_lower');
59+
$query->executeStatement();
60+
}
61+
}

lib/UserBackend.php

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,14 @@ public function deleteUser($uid): bool {
103103
return (bool)$result;
104104
}
105105

106+
public function setInitialEmail(string $uid, string $email): bool {
107+
$query = $this->dbConn->getQueryBuilder();
108+
$query->update('guests_users')
109+
->set('email', $query->createNamedParameter($email))
110+
->where($query->expr()->eq('uid_lower', $query->createNamedParameter(mb_strtolower($uid))));
111+
return (bool)$query->executeStatement();
112+
}
113+
106114
/**
107115
* Change the password of a user
108116
*/
@@ -231,14 +239,43 @@ public function getDisplayNames($search = '', $limit = null, $offset = null): ar
231239
return $displayNames;
232240
}
233241

242+
/**
243+
* Get a list of all users with their email and display name
244+
*
245+
* @return array<string, array{email: string, displayname: string}>
246+
*/
247+
public function getAllGuestAccounts(?int $limit = null, ?int $offset = null): array {
248+
if (!$this->allowListing) {
249+
return [];
250+
}
251+
252+
$query = $this->dbConn->getQueryBuilder();
253+
$query->select('uid', 'email', 'displayname')
254+
->from('guests_users')
255+
->orderBy('uid_lower', 'ASC')
256+
->setMaxResults($limit)
257+
->setFirstResult($offset);
258+
259+
$result = $query->executeQuery();
260+
$users = [];
261+
while ($row = $result->fetch()) {
262+
$users[(string)$row['uid']] = [
263+
'email' => (string)$row['email'],
264+
'displayname' => (string)$row['displayname'],
265+
];
266+
}
267+
268+
return $users;
269+
}
270+
234271
/**
235272
* Check if the password is correct without logging in the user
236273
* returns the user id or false
237274
*
238275
* @return string|false
239276
*/
240277
public function checkPassword(string $loginName, string $password) {
241-
if (!str_contains($loginName, '@')) {
278+
if (!$this->potentialGuestUserId($loginName)) {
242279
return false;
243280
}
244281

@@ -275,9 +312,13 @@ public function checkPassword(string $loginName, string $password) {
275312
* @param string $uid the username
276313
*/
277314
private function loadUser($uid): bool {
315+
if (isset($this->cache[$uid]) && $this->cache[$uid] === false) {
316+
return false;
317+
}
318+
278319
// guests $uid could be NULL or ''
279320
// or is not an email anyway
280-
if (!str_contains($uid, '@')) {
321+
if (!$this->potentialGuestUserId($uid)) {
281322
$this->cache[$uid] = false;
282323
return false;
283324
}
@@ -390,7 +431,7 @@ public function getBackendName(): string {
390431
}
391432

392433
public function getRealUID(string $uid): string {
393-
if (!str_contains($uid, '@')) {
434+
if (!$this->potentialGuestUserId($uid)) {
394435
throw new \RuntimeException($uid . ' does not exist');
395436
}
396437

@@ -404,4 +445,16 @@ public function getRealUID(string $uid): string {
404445

405446
return $this->cache[$uid]['uid'];
406447
}
448+
449+
/**
450+
* Guest app user ids are:
451+
* - either email addresses so they need to contain an @
452+
* - lowercase sha256 hashes of email addresses, 64 characters of a-f and 0-9
453+
*
454+
* @param string $userId
455+
* @return bool
456+
*/
457+
protected function potentialGuestUserId(string $userId): bool {
458+
return str_contains($userId, '@') || preg_match('/^[a-f0-9]{64}$/', $userId);
459+
}
407460
}

0 commit comments

Comments
 (0)