Skip to content

Commit 58da18a

Browse files
bordonibordoni
authored
Merge pull request #27 from bordoni/fix/cons-513-username-generation-oom
[CONS-513] fix(sync): derive username collisions from an email hash instead of sequential probing
2 parents 5d6165e + a6aba55 commit 58da18a

3 files changed

Lines changed: 252 additions & 7 deletions

File tree

‎src/WorkOS/Sync/UserSync.php‎

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -642,24 +642,49 @@ public static function get_wp_user_id_by_workos_id( string $workos_id ): ?int {
642642
/**
643643
* Generate a unique username from WorkOS user data.
644644
*
645+
* Uses the email local part as the base and resolves collisions with a
646+
* short hash of the full email instead of probing sequential suffixes —
647+
* popular shared-mailbox local parts (info, sales, …) build chains deep
648+
* enough that a sequential probe walks thousands of users and exhausts
649+
* memory. Hashing keeps the lookup count constant regardless of chain
650+
* depth, and the same email always derives the same username.
651+
*
645652
* @param array $workos_user WorkOS user data.
646653
*
647654
* @return string
648655
*/
649656
private static function generate_username( array $workos_user ): string {
650-
$email = $workos_user['email'] ?? '';
651-
$base = sanitize_user( strtok( $email, '@' ), true );
657+
$email = strtolower( trim( (string) ( $workos_user['email'] ?? '' ) ) );
658+
$base = sanitize_user( explode( '@', $email, 2 )[0], true );
652659

653660
if ( ! $base ) {
654661
$base = 'workos_user';
655662
}
656663

657-
$username = $base;
658-
$counter = 1;
664+
// Cap so the widest suffix ('_' + 12 hex) fits user_login's varchar(60).
665+
$base = substr( $base, 0, 47 );
666+
667+
if ( ! username_exists( $base ) ) {
668+
return $base;
669+
}
670+
671+
$hash = hash( 'sha256', $email );
672+
$username = $base . '_' . substr( $hash, 0, 5 );
673+
674+
if ( ! username_exists( $username ) ) {
675+
return $username;
676+
}
677+
678+
$username = $base . '_' . substr( $hash, 0, 12 );
679+
680+
// ~2^-48 territory. The generator is deterministic, so recovery must
681+
// change the input — salt with an attempt counter, never re-roll.
682+
for ( $attempt = 1; $attempt <= 3; $attempt++ ) {
683+
if ( ! username_exists( $username ) ) {
684+
return $username;
685+
}
659686

660-
while ( username_exists( $username ) ) {
661-
$username = $base . '_' . $counter;
662-
++$counter;
687+
$username = $base . '_' . substr( hash( 'sha256', $email . '|' . $attempt ), 0, 12 );
663688
}
664689

665690
return $username;

‎tests/wpunit/UserSyncFindOrCreateTest.php‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,4 +242,21 @@ public function test_generates_unique_username_on_conflict(): void {
242242
$this->assertNotSame( 'conflicting', $result->user_login );
243243
$this->assertStringStartsWith( 'conflicting', $result->user_login );
244244
}
245+
246+
/**
247+
* Test the created username carries the deterministic email-hash suffix on conflict.
248+
*/
249+
public function test_creates_user_with_hash_suffix_when_local_part_taken(): void {
250+
self::factory()->user->create( [ 'user_login' => 'info', 'user_email' => 'other-info@example.com' ] );
251+
252+
$result = UserSync::find_or_create_wp_user(
253+
[
254+
'id' => 'user_info_hash_test',
255+
'email' => 'info@acme-widgets.com',
256+
]
257+
);
258+
259+
$this->assertInstanceOf( \WP_User::class, $result );
260+
$this->assertSame( 'info_48f25', $result->user_login );
261+
}
245262
}

‎tests/wpunit/UserSyncGenerateUsernameTest.php‎

Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
<?php
2+
/**
3+
* Tests for UserSync::generate_username().
4+
*
5+
* @package WorkOS\Tests\Wpunit
6+
*/
7+
8+
namespace WorkOS\Tests\Wpunit;
9+
10+
use Closure;
11+
use lucatume\WPBrowser\TestCase\WPTestCase;
12+
use ReflectionMethod;
13+
use WorkOS\Sync\UserSync;
14+
15+
/**
16+
* Username generation tests.
17+
*
18+
* The generator must derive usernames in O(1) lookups regardless of how many
19+
* users share the same email local part — the previous sequential probe
20+
* (info, info_1, info_2, …) walked the whole chain and exhausted memory on
21+
* deep chains (CONS-513). Expected hash values are precomputed sha256 hex
22+
* digests of the lowercased email (or email + '|' + attempt for salted
23+
* retries), independent of the implementation.
24+
*/
25+
class UserSyncGenerateUsernameTest extends WPTestCase {
26+
27+
/**
28+
* Set up each test.
29+
*/
30+
public function setUp(): void {
31+
parent::setUp();
32+
33+
remove_all_actions( 'user_register' );
34+
}
35+
36+
/**
37+
* Invoke the private username generator via reflection.
38+
*
39+
* @param string $email Email to derive a username from.
40+
*
41+
* @return string Generated username.
42+
*/
43+
private function generate( string $email ): string {
44+
$method = new ReflectionMethod( UserSync::class, 'generate_username' );
45+
$method->setAccessible( true );
46+
47+
return $method->invoke( null, [ 'email' => $email ] );
48+
}
49+
50+
/**
51+
* Create a user occupying the given login.
52+
*
53+
* @param string $login Login to occupy.
54+
*/
55+
private function seed_login( string $login ): void {
56+
self::factory()->user->create(
57+
[
58+
'user_login' => $login,
59+
'user_email' => md5( $login ) . '@seed.test',
60+
]
61+
);
62+
}
63+
64+
/**
65+
* Collision-ladder scenarios.
66+
*
67+
* Each case seeds the usernames the email's derivation collides with,
68+
* then states the expected outcome — reading top to bottom walks the
69+
* full ladder: bare base → 5-hex suffix → 12-hex widening → salted
70+
* retries.
71+
*
72+
* @return array<string, array{0: Closure, 1: string, 2: string}>
73+
*/
74+
public function username_scenario_provider(): array {
75+
$seed = static function ( string ...$logins ): Closure {
76+
return static function ( self $test ) use ( $logins ): void {
77+
foreach ( $logins as $login ) {
78+
$test->seed_login( $login );
79+
}
80+
};
81+
};
82+
83+
return [
84+
'bare local part when base is free' => [
85+
$seed(),
86+
'info@acme-widgets.com',
87+
'info',
88+
],
89+
'5-hex email hash suffix when base is taken' => [
90+
$seed( 'info' ),
91+
'info@acme-widgets.com',
92+
'info_48f25',
93+
],
94+
'widened 12-hex suffix when 5-hex name taken' => [
95+
$seed( 'info', 'info_48f25' ),
96+
'info@acme-widgets.com',
97+
'info_48f257791ee0',
98+
],
99+
'first salted retry when widened name taken' => [
100+
$seed( 'info', 'info_48f25', 'info_48f257791ee0' ),
101+
'info@acme-widgets.com',
102+
'info_b1dc4def5ca9',
103+
],
104+
'second salted retry when first salt taken' => [
105+
$seed( 'info', 'info_48f25', 'info_48f257791ee0', 'info_b1dc4def5ca9' ),
106+
'info@acme-widgets.com',
107+
'info_023b84c3d9b3',
108+
],
109+
'long local part capped to 47 chars' => [
110+
$seed(),
111+
'international-wholesale-distribution-and-logistics-coordination@globex.test',
112+
'international-wholesale-distribution-and-logist',
113+
],
114+
'widened suffix on capped base lands on 60' => [
115+
$seed(
116+
'international-wholesale-distribution-and-logist',
117+
'international-wholesale-distribution-and-logist_6a3ae'
118+
),
119+
'international-wholesale-distribution-and-logistics-coordination@globex.test',
120+
'international-wholesale-distribution-and-logist_6a3aeb9adeab',
121+
],
122+
'workos_user fallback when local part empty' => [
123+
$seed(),
124+
'@example.com',
125+
'workos_user',
126+
],
127+
];
128+
}
129+
130+
/**
131+
* Test the generator walks the collision ladder deterministically.
132+
*
133+
* @dataProvider username_scenario_provider
134+
*
135+
* @param Closure $arrange Seeds the usernames the scenario collides with.
136+
* @param string $email Email to derive a username from.
137+
* @param string $expected Expected username.
138+
*/
139+
public function test_derives_expected_username( Closure $arrange, string $email, string $expected ): void {
140+
$arrange( $this );
141+
142+
$username = $this->generate( $email );
143+
144+
$this->assertSame( $expected, $username );
145+
$this->assertLessThanOrEqual( 60, strlen( $username ) );
146+
}
147+
148+
/**
149+
* Test the same email derives the identical username across runs.
150+
*/
151+
public function test_same_email_derives_identical_username_across_runs(): void {
152+
$this->seed_login( 'info' );
153+
154+
$first = $this->generate( 'info@acme-widgets.com' );
155+
$second = $this->generate( 'info@acme-widgets.com' );
156+
157+
$this->assertSame( $first, $second );
158+
}
159+
160+
/**
161+
* Test lookup count stays constant no matter how deep the existing chain is.
162+
*
163+
* This is the regression test for the production OOM: the old sequential
164+
* probe performed one lookup per existing info_* user. With 150 seeded
165+
* users it would need 152 lookups; the generator must need exactly 2.
166+
*/
167+
public function test_lookup_count_is_constant_for_deep_username_chains(): void {
168+
$this->seed_login( 'info' );
169+
for ( $i = 1; $i <= 150; $i++ ) {
170+
$this->seed_login( 'info_' . $i );
171+
}
172+
173+
$lookups = 0;
174+
$counter = static function ( $user_id ) use ( &$lookups ) {
175+
++$lookups;
176+
177+
return $user_id;
178+
};
179+
180+
add_filter( 'username_exists', $counter );
181+
$username = $this->generate( 'info@acme-widgets.com' );
182+
remove_filter( 'username_exists', $counter );
183+
184+
$this->assertSame( 'info_48f25', $username );
185+
$this->assertSame( 2, $lookups );
186+
}
187+
188+
/**
189+
* Test emails sharing a truncated base still derive distinct usernames.
190+
*
191+
* The hash is computed from the full email, never the truncated base, so
192+
* truncation must not create collisions.
193+
*/
194+
public function test_truncated_bases_still_get_distinct_usernames(): void {
195+
$this->seed_login( 'international-wholesale-distribution-and-logist' );
196+
197+
$first = $this->generate( 'international-wholesale-distribution-and-logistics-coordination@globex.test' );
198+
$second = $this->generate( 'international-wholesale-distribution-and-logistics-department@globex.test' );
199+
200+
$this->assertSame( 'international-wholesale-distribution-and-logist_6a3ae', $first );
201+
$this->assertSame( 'international-wholesale-distribution-and-logist_1c20f', $second );
202+
}
203+
}

0 commit comments

Comments
 (0)