Changeset View
Changeset View
Standalone View
Standalone View
src/app/SignupCode.php
Show First 20 Lines • Show All 93 Lines • ▼ Show 20 Lines | class SignupCode extends Model | ||||
/** | /** | ||||
* Generate a short code (for human). | * Generate a short code (for human). | ||||
* | * | ||||
* @return string | * @return string | ||||
*/ | */ | ||||
public static function generateShortCode(): string | public static function generateShortCode(): string | ||||
{ | { | ||||
$code_length = env('SIGNUP_CODE_LENGTH', self::SHORTCODE_LENGTH); | $codeLength = env('SIGNUP_CODE_LENGTH', self::SHORTCODE_LENGTH); | ||||
return \App\Utils::randStr($code_length); | $allegedlyUnique = \App\Utils::randStr($codeLength); | ||||
while ($code = $this->where('short_code', $allegedlyUnique)->first()) { | |||||
if ($code->isExpired()) { | |||||
mollekopf: Doesn't this check mean that if you accidentally generate a collision with an expired code it's… | |||||
vanmeeuwenAuthorUnsubmitted Done Inline ActionsNo, but it at least prevents this; MariaDB [kolabdev]> select count(*) as count, short_code from signup_codes where expires_at > NOW() group by short_code order by count desc limit 0,10; +-------+------------+ | count | short_code | +-------+------------+ | 3 | ***** | | 3 | ***** | | 3 | ***** | | 2 | ***** | (...) 10 rows in set (0.077 sec) vanmeeuwen: No, but it at least prevents this;
```
MariaDB [kolabdev]> select count(*) as count… | |||||
mollekopfUnsubmitted Not Done Inline ActionsTo me this code reads like:
Maybe it's okay to reuse expired shortcodes because they're about to be purged anyways? In either case you shouldn't end up with duplicates, so at best this is an optimization (that may or may not be necessary) and at worst it's masking an error in the uniqueness guarantee. mollekopf: To me this code reads like:
* Generate a supposedly unique shortcode
* Check if it's unique
*… | |||||
break; | |||||
} | |||||
$allegedlyUnique = \App\Utils::randStr($codeLength); | |||||
} | |||||
return $allegedlyUnique; | |||||
} | } | ||||
} | } |
Doesn't this check mean that if you accidentally generate a collision with an expired code it's reused?
So it seems to me you shouldn't check for isExpired and always generate again if there is a collision.