Skip to content

Commit f4b03fc

Browse files
CarlSchwanCarlSchwan
committed
feat(user_oidc): Add provider_id/sub columns and getByProviderAndSub()
Store the provider and OIDC subject claim a user was provisioned from alongside the (possibly hashed) user_id, and look accounts up by this (provider_id, sub) pair first in getOrCreate(). This keeps a provider's own attribute drift (e.g. the mapped uid attribute changing) from forking the account into a second, empty one. Existing rows are backfilled opportunistically on next login, since the original sub is not recoverable from a one-way hashed user_id. Signed-off-by: Carl Schwan <carl@carlschwan.eu>
1 parent 7583eee commit f4b03fc

6 files changed

Lines changed: 207 additions & 5 deletions

File tree

‎lib/Controller/OcsApiController.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public function __construct(
3434
* Create or update a user for a backend provider.
3535
*
3636
* @param int $providerId Numeric ID of the provider backend
37-
* @param string $userId Provider-specific user identifier
37+
* @param non-empty-string $userId Provider-specific user identifier
3838
* @param string|null $displayName Optional display name to set for the user
3939
* @param string|null $email Optional email address to set for the user
4040
* @param string|null $quota Optional quota value to set for the user

‎lib/Db/User.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@
1616
* @method \void setUserId(string $userId)
1717
* @method \string getDisplayName()
1818
* @method \void setDisplayName(string $displayName)
19+
* @method \int|null getProviderId()
20+
* @method \void setProviderId(?int $providerId)
21+
* @method \string|null getSub()
22+
* @method \void setSub(?string $sub)
1923
*/
2024
class User extends Entity {
2125

@@ -25,8 +29,24 @@ class User extends Entity {
2529
/** @var string */
2630
protected $displayName;
2731

32+
/**
33+
* The provider this user was provisioned from, kept alongside the
34+
* (possibly hashed) userId so the same person can be recognized again
35+
* independently of the user_id-generation setting.
36+
* @var int|null
37+
*/
38+
protected $providerId;
39+
40+
/**
41+
* The OIDC subject claim this user was provisioned from. See $providerId.
42+
* @var string|null
43+
*/
44+
protected $sub;
45+
2846
public function __construct() {
2947
$this->addType('userId', Types::STRING);
3048
$this->addType('displayName', Types::STRING);
49+
$this->addType('providerId', Types::INTEGER);
50+
$this->addType('sub', Types::STRING);
3151
}
3252
}

‎lib/Db/UserMapper.php‎

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,22 +152,69 @@ public function userExists(string $uid): bool {
152152
}
153153
}
154154

155+
/**
156+
* @param non-empty-string $sub Sub of the user hashed if it exceed 256 characters
157+
* @throws \OCP\AppFramework\Db\DoesNotExistException
158+
* @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
159+
*/
160+
protected function getByProviderAndSub(int $providerId, string $sub): User {
161+
$qb = $this->db->getQueryBuilder();
162+
$qb->select('*')
163+
->from($this->getTableName())
164+
->where($qb->expr()->eq('provider_id', $qb->createNamedParameter($providerId, IQueryBuilder::PARAM_INT)))
165+
->andWhere($qb->expr()->eq('sub', $qb->createNamedParameter($sub, IQueryBuilder::PARAM_STR)));
166+
167+
/** @var User $user */
168+
$user = $this->findEntity($qb);
169+
$this->userCache->set($user->getUserId(), $user);
170+
return $user;
171+
}
172+
173+
/**
174+
* @param non-empty-string $sub
175+
*/
155176
public function getOrCreate(int $providerId, string $sub, bool $id4me = false): User {
177+
// the sub is the stable identifier we want to keep around, so guard it
178+
// against the column length the same way the userId is further below
179+
$storedSub = strlen($sub) > 256 ? hash('sha256', $sub) : $sub;
180+
181+
try {
182+
// look this identity up by its immutable (provider, sub) pair
183+
// first: if it has been provisioned before, this always returns
184+
// its existing account, even if the uid-generation settings
185+
// changed since - this is what keeps a provider's own attribute
186+
// drift from forking the account into a second, empty one
187+
return $this->getByProviderAndSub($providerId, $storedSub);
188+
} catch (IMapperException $e) {
189+
// not seen under this provider+sub yet, fall through below
190+
}
191+
156192
$userId = $this->idService->getId($providerId, $sub, $id4me);
157193

158194
if (strlen($userId) > 64) {
159195
$userId = hash('sha256', $userId);
160196
}
161197

162198
try {
163-
return $this->getUser($userId);
199+
$user = $this->getUser($userId);
200+
if ($user->getProviderId() === null || $user->getSub() === null) {
201+
// backfill accounts that were provisioned before these columns
202+
// existed, so their next login takes the fast path above
203+
$user->setProviderId($providerId);
204+
$user->setSub($storedSub);
205+
$user = $this->update($user);
206+
$this->userCache->set($userId, $user);
207+
}
208+
return $user;
164209
} catch (IMapperException $e) {
165210
// just ignore and continue
166211
}
167212

168213
$user = new User();
169214
$user->setUserId($userId);
170215
$user->setDisplayName('');
216+
$user->setProviderId($providerId);
217+
$user->setSub($storedSub);
171218
$user = $this->insert($user);
172219
$this->userCache->set($userId, $user);
173220
return $user;

‎lib/Migration/Version081100Date20260824120000.php‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\UserOIDC\Migration;
11+
12+
use Closure;
13+
use OCP\DB\ISchemaWrapper;
14+
use OCP\DB\Types;
15+
use OCP\Migration\Attributes\AddColumn;
16+
use OCP\Migration\Attributes\ColumnType;
17+
use OCP\Migration\IOutput;
18+
use OCP\Migration\SimpleMigrationStep;
19+
20+
//#[AddColumn(table: 'user_oidc', name: 'provider_id', type: ColumnType::INTEGER, description: 'Store the id of the provider for this user')]
21+
//#[AddColumn(table: 'user_oidc', name: 'sub', type: ColumnType::STRING, description: 'Store the sub for this user')]
22+
class Version081100Date20260824120000 extends SimpleMigrationStep {
23+
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
24+
/** @var ISchemaWrapper $schema */
25+
$schema = $schemaClosure();
26+
$changed = false;
27+
28+
$table = $schema->getTable('user_oidc');
29+
if (!$table->hasColumn('provider_id')) {
30+
$table->addColumn('provider_id', Types::INTEGER, [
31+
'notnull' => false,
32+
'length' => 4,
33+
]);
34+
$changed = true;
35+
}
36+
if (!$table->hasColumn('sub')) {
37+
$table->addColumn('sub', Types::STRING, [
38+
'notnull' => false,
39+
'length' => 256,
40+
]);
41+
$changed = true;
42+
}
43+
if (!$table->hasIndex('user_oidc_prov_sub')) {
44+
// unique: this is now the primary lookup key for an existing
45+
// identity, not just an auxiliary index
46+
$table->addUniqueIndex(['provider_id', 'sub'], 'user_oidc_prov_sub');
47+
$changed = true;
48+
}
49+
50+
return $changed ? $schema : null;
51+
}
52+
}

‎openapi.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1567,7 +1567,8 @@
15671567
},
15681568
"userId": {
15691569
"type": "string",
1570-
"description": "Provider-specific user identifier"
1570+
"description": "Provider-specific user identifier",
1571+
"minLength": 1
15711572
},
15721573
"displayName": {
15731574
"type": "string",

‎tests/unit/Db/UserMapperTest.php‎

Lines changed: 84 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
declare(strict_types=1);
99

10+
use OCA\UserOIDC\Db\User;
1011
use OCA\UserOIDC\Db\UserMapper;
1112
use OCA\UserOIDC\Service\LocalIdService;
1213
use OCP\AppFramework\Db\DoesNotExistException;
@@ -39,7 +40,7 @@ public function setUp(): void {
3940
$this->db = $this->createMock(IDBConnection::class);
4041
$this->userMapper = $this->getMockBuilder(UserMapper::class)
4142
->setConstructorArgs([$this->db, $this->idService, $this->config])
42-
->onlyMethods(['getUser', 'insert'])
43+
->onlyMethods(['getUser', 'getByProviderAndSub', 'insert', 'update'])
4344
->getMock();
4445
}
4546

@@ -67,6 +68,10 @@ public static function dataCreate(): array {
6768

6869
#[DataProvider('dataCreate')]
6970
public function testCreate(int $providerId, string $sub, string $generatedId, bool $id4me, string $expected): void {
71+
$this->userMapper->expects(self::once())
72+
->method('getByProviderAndSub')
73+
->willThrowException(new DoesNotExistException('No user'));
74+
7075
$this->idService->expects(self::once())->method('getId')->with($providerId, $sub, $id4me)->willReturn($generatedId);
7176

7277
$this->userMapper->expects(self::once())
@@ -78,7 +83,84 @@ public function testCreate(int $providerId, string $sub, string $generatedId, bo
7883
->willReturnCallback(function ($arg) {
7984
return $arg;
8085
});
86+
$this->userMapper->expects(self::never())->method('update');
87+
88+
$user = $this->userMapper->getOrCreate($providerId, $sub, $id4me);
89+
Assert::assertEquals($expected, $user->getUserId());
90+
Assert::assertSame($providerId, $user->getProviderId());
91+
Assert::assertSame($sub, $user->getSub());
92+
}
93+
94+
public function testCreateHashesOverlongSub(): void {
95+
$longSub = str_repeat('a', 300);
96+
97+
$this->userMapper->expects(self::once())
98+
->method('getByProviderAndSub')
99+
->willThrowException(new DoesNotExistException('No user'));
100+
101+
$this->idService->expects(self::once())->method('getId')->willReturn('short-user-id');
102+
103+
$this->userMapper->expects(self::once())
104+
->method('getUser')
105+
->willThrowException(new DoesNotExistException('No user'));
106+
107+
$this->userMapper->expects(self::once())
108+
->method('insert')
109+
->willReturnCallback(function ($arg) {
110+
return $arg;
111+
});
112+
113+
$user = $this->userMapper->getOrCreate(1, $longSub);
114+
Assert::assertSame(hash('sha256', $longSub), $user->getSub());
115+
}
116+
117+
public function testGetOrCreateBackfillsExistingUserWithoutStableIdentifier(): void {
118+
// simulates an account provisioned before provider_id/sub existed:
119+
// not found by that pair yet, but its computed uid already exists
120+
$existing = new User();
121+
$existing->setUserId('existing-user');
122+
$existing->setDisplayName('Existing User');
123+
124+
$this->userMapper->expects(self::once())
125+
->method('getByProviderAndSub')
126+
->willThrowException(new DoesNotExistException('No user'));
127+
128+
$this->idService->expects(self::once())->method('getId')->willReturn('existing-user');
129+
130+
$this->userMapper->expects(self::once())
131+
->method('getUser')
132+
->willReturn($existing);
133+
134+
$this->userMapper->expects(self::once())
135+
->method('update')
136+
->willReturnCallback(function ($arg) {
137+
return $arg;
138+
});
139+
$this->userMapper->expects(self::never())->method('insert');
140+
141+
$user = $this->userMapper->getOrCreate(5, 'the-sub');
142+
Assert::assertSame(5, $user->getProviderId());
143+
Assert::assertSame('the-sub', $user->getSub());
144+
}
145+
146+
public function testGetOrCreateReturnsExistingUserByProviderAndSubWithoutComputingAUid(): void {
147+
$existing = new User();
148+
$existing->setUserId('existing-user');
149+
$existing->setDisplayName('Existing User');
150+
$existing->setProviderId(5);
151+
$existing->setSub('the-sub');
152+
153+
$this->userMapper->expects(self::once())
154+
->method('getByProviderAndSub')
155+
->with(5, 'the-sub')
156+
->willReturn($existing);
157+
158+
$this->idService->expects(self::never())->method('getId');
159+
$this->userMapper->expects(self::never())->method('getUser');
160+
$this->userMapper->expects(self::never())->method('update');
161+
$this->userMapper->expects(self::never())->method('insert');
81162

82-
Assert::assertEquals($expected, $this->userMapper->getOrCreate($providerId, $sub, $id4me)->getUserId());
163+
$user = $this->userMapper->getOrCreate(5, 'the-sub');
164+
Assert::assertSame($existing, $user);
83165
}
84166
}

0 commit comments

Comments
 (0)