From 845c8b3d858eb5d7075308d935d0435af619fddd Mon Sep 17 00:00:00 2001 From: Shish Date: Sat, 31 Aug 2024 20:35:13 +0100 Subject: [PATCH] [core] Make User::by_name / User::by_id not-null Nearly everywhere that these functions are called, the result is assumed to be not-null, and a null will break things --- core/cli_app.php | 6 +-- core/imageboard/image.php | 1 - core/polyfills.php | 4 +- core/testcase.php | 6 +-- core/user.php | 83 ++++++++++++++++--------------------- core/userclass.php | 2 +- core/util.php | 1 - ext/alias_editor/main.php | 2 + ext/auto_tagger/main.php | 2 + ext/danbooru_api/main.php | 12 +++--- ext/numeric_score/main.php | 10 ----- ext/numeric_score/test.php | 4 +- ext/post_owner/main.php | 6 +-- ext/replace_file/main.php | 10 ++--- ext/setup/main.php | 2 +- ext/source_history/main.php | 9 +--- ext/tag_history/main.php | 9 +--- ext/user/main.php | 50 ++++++++++------------ ext/user/test.php | 4 +- ext/user_config/main.php | 1 - 20 files changed, 88 insertions(+), 136 deletions(-) diff --git a/core/cli_app.php b/core/cli_app.php index 47fec20b..b6c2c5cd 100644 --- a/core/cli_app.php +++ b/core/cli_app.php @@ -38,11 +38,7 @@ class CliApp extends \Symfony\Component\Console\Application if ($input->hasParameterOption(['--user', '-u'])) { $name = $input->getParameterOption(['--user', '-u']); $user = User::by_name($name); - if (is_null($user)) { - die("Unknown user '$name'\n"); - } else { - send_event(new UserLoginEvent($user)); - } + send_event(new UserLoginEvent($user)); } $log_level = SCORE_LOG_WARNING; diff --git a/core/imageboard/image.php b/core/imageboard/image.php index 7a8e0b99..a1ee0dbb 100644 --- a/core/imageboard/image.php +++ b/core/imageboard/image.php @@ -265,7 +265,6 @@ class Image implements \ArrayAccess public function get_owner(): User { $user = User::by_id($this->owner_id); - assert(!is_null($user)); return $user; } diff --git a/core/polyfills.php b/core/polyfills.php index 30ce6b97..075cc690 100644 --- a/core/polyfills.php +++ b/core/polyfills.php @@ -628,7 +628,9 @@ function validate_input(array $inputs): array if (in_array('user_id', $flags)) { $id = int_escape($value); if (in_array('exists', $flags)) { - if (is_null(User::by_id($id))) { + try { + User::by_id($id); + } catch (UserNotFound $e) { throw new InvalidInput("User #$id does not exist"); } } diff --git a/core/testcase.php b/core/testcase.php index 3bee4163..5e081484 100644 --- a/core/testcase.php +++ b/core/testcase.php @@ -241,18 +241,18 @@ if (class_exists("\\PHPUnit\\Framework\\TestCase")) { // user things protected static function log_in_as_admin(): void { - send_event(new UserLoginEvent(User::by_name_ex(self::$admin_name))); + send_event(new UserLoginEvent(User::by_name(self::$admin_name))); } protected static function log_in_as_user(): void { - send_event(new UserLoginEvent(User::by_name_ex(self::$user_name))); + send_event(new UserLoginEvent(User::by_name(self::$user_name))); } protected static function log_out(): void { global $config; - send_event(new UserLoginEvent(User::by_id_ex($config->get_int("anon_id", 0)))); + send_event(new UserLoginEvent(User::by_id($config->get_int("anon_id", 0)))); } // post things diff --git a/core/user.php b/core/user.php index 26d86acb..dec59c4f 100644 --- a/core/user.php +++ b/core/user.php @@ -85,8 +85,9 @@ class User global $cache, $config, $database; $user = $cache->get("user-session-obj:$name-$session"); if (is_null($user)) { - $user_by_name = User::by_name($name); - if (is_null($user_by_name)) { + try { + $user_by_name = User::by_name($name); + } catch (UserNotFound $e) { return null; } if ($user_by_name->get_session_id() === $session) { @@ -97,7 +98,7 @@ class User return $user; } - public static function by_id(int $id): ?User + public static function by_id(int $id): User { global $cache, $database; if ($id === 1) { @@ -110,72 +111,55 @@ class User if ($id === 1) { $cache->set('user-id:'.$id, $row, 600); } - return is_null($row) ? null : new User($row); - } - - public static function by_id_ex(int $id): User - { - $u = User::by_id($id); - if (is_null($u)) { + if (is_null($row)) { throw new UserNotFound("Can't find any user with ID $id"); - } else { - return $u; } + return new User($row); } #[Query(name: "user")] - public static function by_name(string $name): ?User + public static function by_name(string $name): User { global $database; $row = $database->get_row("SELECT * FROM users WHERE LOWER(name) = LOWER(:name)", ["name" => $name]); - return is_null($row) ? null : new User($row); - } - - public static function by_name_ex(string $name): User - { - $u = User::by_name($name); - if (is_null($u)) { + if (is_null($row)) { throw new UserNotFound("Can't find any user named $name"); } else { - return $u; + return new User($row); } } public static function name_to_id(string $name): int { - $u = User::by_name($name); - if (is_null($u)) { - throw new UserNotFound("Can't find any user named $name"); - } else { - return $u->id; - } + return User::by_name($name)->id; } - public static function by_name_and_pass(string $name, string $pass): ?User + public static function by_name_and_pass(string $name, string $pass): User { - $my_user = User::by_name($name); - - // If user tried to log in as "foo bar" and failed, try "foo_bar" - if (!$my_user && str_contains($name, " ")) { - $my_user = User::by_name(str_replace(" ", "_", $name)); + try { + $my_user = User::by_name($name); + } catch (UserNotFound $e) { + // If user tried to log in as "foo bar" and failed, try "foo_bar" + try { + $my_user = User::by_name(str_replace(" ", "_", $name)); + } catch (UserNotFound $e) { + log_warning("core-user", "Failed to log in as $name (Invalid username)"); + throw $e; + } } - if ($my_user) { - if ($my_user->passhash == md5(strtolower($name) . $pass)) { - log_info("core-user", "Migrating from md5 to bcrypt for $name"); - $my_user->set_password($pass); - } - assert(!is_null($my_user->passhash)); - if (password_verify($pass, $my_user->passhash)) { - log_info("core-user", "Logged in as $name ({$my_user->class->name})"); - return $my_user; - } else { - log_warning("core-user", "Failed to log in as $name (Invalid password)"); - } + if ($my_user->passhash == md5(strtolower($name) . $pass)) { + log_info("core-user", "Migrating from md5 to bcrypt for $name"); + $my_user->set_password($pass); + } + assert(!is_null($my_user->passhash)); + if (password_verify($pass, $my_user->passhash)) { + log_info("core-user", "Logged in as $name ({$my_user->class->name})"); + return $my_user; } else { - log_warning("core-user", "Failed to log in as $name (Invalid username)"); + log_warning("core-user", "Failed to log in as $name (Invalid password)"); + throw new UserNotFound("Can't find anybody with that username and password"); } - return null; } @@ -203,8 +187,11 @@ class User public function set_name(string $name): void { global $database; - if (User::by_name($name)) { + try { + User::by_name($name); throw new InvalidInput("Desired username is already in use"); + } catch (UserNotFound $e) { + // if user is not found, we're good } $old_name = $this->name; $this->name = $name; diff --git a/core/userclass.php b/core/userclass.php index 530b9454..1f363100 100644 --- a/core/userclass.php +++ b/core/userclass.php @@ -17,7 +17,7 @@ class UserClass public static array $known_classes = []; #[Field] - public ?string $name = null; + public string $name; public ?UserClass $parent = null; /** @var array */ diff --git a/core/util.php b/core/util.php index a3ec277b..576e8a0d 100644 --- a/core/util.php +++ b/core/util.php @@ -747,7 +747,6 @@ function _get_user(): User if (is_null($my_user)) { $my_user = User::by_id($config->get_int("anon_id", 0)); } - assert(!is_null($my_user)); return $my_user; } diff --git a/ext/alias_editor/main.php b/ext/alias_editor/main.php index 1957ff66..bf739a69 100644 --- a/ext/alias_editor/main.php +++ b/ext/alias_editor/main.php @@ -175,6 +175,8 @@ class AliasEditor extends Extension foreach (explode("\n", $csv) as $line) { $parts = str_getcsv($line); if (count($parts) == 2) { + assert(is_string($parts[0])); + assert(is_string($parts[1])); send_event(new AddAliasEvent($parts[0], $parts[1])); $i++; } diff --git a/ext/auto_tagger/main.php b/ext/auto_tagger/main.php index eca01d14..2fefc0c5 100644 --- a/ext/auto_tagger/main.php +++ b/ext/auto_tagger/main.php @@ -187,6 +187,8 @@ class AutoTagger extends Extension foreach (explode("\n", $csv) as $line) { $parts = str_getcsv($line); if (count($parts) == 2) { + assert(is_string($parts[0])); + assert(is_string($parts[1])); send_event(new AddAutoTagEvent($parts[0], $parts[1])); $i++; } diff --git a/ext/danbooru_api/main.php b/ext/danbooru_api/main.php index 3be1fb21..5a275d99 100644 --- a/ext/danbooru_api/main.php +++ b/ext/danbooru_api/main.php @@ -80,14 +80,12 @@ class DanbooruApi extends Extension if (isset($_REQUEST['login']) && isset($_REQUEST['password'])) { // Get this user from the db, if it fails the user becomes anonymous // Code borrowed from /ext/user - $name = $_REQUEST['login']; - $pass = $_REQUEST['password']; - $duser = User::by_name_and_pass($name, $pass); - if (!is_null($duser)) { - $user = $duser; - } else { + try { + $name = $_REQUEST['login']; + $pass = $_REQUEST['password']; + $user = User::by_name_and_pass($name, $pass); + } catch (UserNotFound $e) { $user = User::by_id($config->get_int("anon_id", 0)); - assert(!is_null($user)); } send_event(new UserLoginEvent($user)); } diff --git a/ext/numeric_score/main.php b/ext/numeric_score/main.php index 187f6092..6352f7b6 100644 --- a/ext/numeric_score/main.php +++ b/ext/numeric_score/main.php @@ -327,22 +327,12 @@ class NumericScore extends Extension $event->add_querylet(new Querylet("numeric_score $cmp $score")); } elseif (preg_match("/^upvoted_by[=|:](.*)$/i", $event->term, $matches)) { $duser = User::by_name($matches[1]); - if (is_null($duser)) { - throw new SearchTermParseException( - "Can't find the user named ".html_escape($matches[1]) - ); - } $event->add_querylet(new Querylet( "images.id in (SELECT image_id FROM numeric_score_votes WHERE user_id=:ns_user_id AND score=1)", ["ns_user_id" => $duser->id] )); } elseif (preg_match("/^downvoted_by[=|:](.*)$/i", $event->term, $matches)) { $duser = User::by_name($matches[1]); - if (is_null($duser)) { - throw new SearchTermParseException( - "Can't find the user named ".html_escape($matches[1]) - ); - } $event->add_querylet(new Querylet( "images.id in (SELECT image_id FROM numeric_score_votes WHERE user_id=:ns_user_id AND score=-1)", ["ns_user_id" => $duser->id] diff --git a/ext/numeric_score/test.php b/ext/numeric_score/test.php index 77eba21d..eb68ec38 100644 --- a/ext/numeric_score/test.php +++ b/ext/numeric_score/test.php @@ -50,10 +50,10 @@ class NumericScoreTest extends ShimmiePHPUnitTestCase $this->assertEquals(404, $page->code); # test errors - $this->assertException(SearchTermParseException::class, function () { + $this->assertException(UserNotFound::class, function () { $this->get_page("post/list/upvoted_by=asdfasdf/1"); }); - $this->assertException(SearchTermParseException::class, function () { + $this->assertException(UserNotFound::class, function () { $this->get_page("post/list/downvoted_by=asdfasdf/1"); }); diff --git a/ext/post_owner/main.php b/ext/post_owner/main.php index 3c871292..1ecdaf17 100644 --- a/ext/post_owner/main.php +++ b/ext/post_owner/main.php @@ -28,11 +28,7 @@ class PostOwner extends Extension $owner = $event->get_param('owner'); if ($user->can(Permissions::EDIT_IMAGE_OWNER) && !is_null($owner)) { $owner_ob = User::by_name($owner); - if (!is_null($owner_ob)) { - send_event(new OwnerSetEvent($event->image, $owner_ob)); - } else { - throw new UserNotFound("Error: No user with that name was found."); - } + send_event(new OwnerSetEvent($event->image, $owner_ob)); } } diff --git a/ext/replace_file/main.php b/ext/replace_file/main.php index 88f3bb2c..7a37ddfb 100644 --- a/ext/replace_file/main.php +++ b/ext/replace_file/main.php @@ -67,12 +67,10 @@ class ReplaceFile extends Extension $image->remove_image_only(); // Actually delete the old image file from disk $target = warehouse_path(Image::IMAGE_DIR, $event->new_hash); - if (!@copy($event->tmp_filename, $target)) { - $errors = error_get_last(); - throw new ImageReplaceException( - "Failed to copy file from uploads ({$event->tmp_filename}) to archive ($target): ". - "{$errors['type']} / {$errors['message']}" - ); + try { + \Safe\copy($event->tmp_filename, $target); + } catch (\Exception $e) { + throw new ImageReplaceException("Failed to copy file from uploads ({$event->tmp_filename}) to archive ($target): {$e->getMessage()}"); } unlink($event->tmp_filename); diff --git a/ext/setup/main.php b/ext/setup/main.php index a7f74160..5ccb0d4e 100644 --- a/ext/setup/main.php +++ b/ext/setup/main.php @@ -239,7 +239,7 @@ class SetupBlock extends Block public function add_shorthand_int_option(string $name, string $label = null, bool $table_row = false): void { - $val = to_shorthand_int($this->config->get_int($name)); + $val = to_shorthand_int($this->config->get_int($name, 0)); $html = "\n"; $html .= "\n"; diff --git a/ext/source_history/main.php b/ext/source_history/main.php index 065b6758..aa254726 100644 --- a/ext/source_history/main.php +++ b/ext/source_history/main.php @@ -267,13 +267,8 @@ class SourceHistory extends Extension if (!is_null($name)) { $duser = User::by_name($name); - if (is_null($duser)) { - $this->theme->add_status($name, "user not found"); - return; - } else { - $select_code[] = 'user_id = :user_id'; - $select_args['user_id'] = $duser->id; - } + $select_code[] = 'user_id = :user_id'; + $select_args['user_id'] = $duser->id; } if (!is_null($ip)) { diff --git a/ext/tag_history/main.php b/ext/tag_history/main.php index 1af6c08d..91c0c353 100644 --- a/ext/tag_history/main.php +++ b/ext/tag_history/main.php @@ -313,13 +313,8 @@ class TagHistory extends Extension if (!is_null($name)) { $duser = User::by_name($name); - if (is_null($duser)) { - $this->theme->add_status($name, "user not found"); - return; - } else { - $select_code[] = 'user_id = :user_id'; - $select_args['user_id'] = $duser->id; - } + $select_code[] = 'user_id = :user_id'; + $select_args['user_id'] = $duser->id; } if (!is_null($ip)) { diff --git a/ext/user/main.php b/ext/user/main.php index bbff2958..0e32a378 100644 --- a/ext/user/main.php +++ b/ext/user/main.php @@ -90,17 +90,16 @@ class LoginResult public static function login(string $username, string $password): LoginResult { global $config; - $duser = User::by_name_and_pass($username, $password); - if (!is_null($duser)) { + try { + $duser = User::by_name_and_pass($username, $password); return new LoginResult( $duser, $duser->get_session_id(), null ); - } else { - $anon = User::by_id($config->get_int("anon_id", 0)); + } catch (UserNotFound $ex) { return new LoginResult( - $anon, + User::by_id($config->get_int("anon_id", 0)), null, "No user found" ); @@ -310,11 +309,14 @@ class UserPage extends Extension } if ($event->page_matches("user/{name}")) { - $display_user = User::by_name($event->get_arg('name')); - if (!is_null($display_user) && ($display_user->id != $config->get_int("anon_id"))) { + try { + $display_user = User::by_name($event->get_arg('name')); + if ($display_user->id == $config->get_int("anon_id")) { + throw new UserNotFound("No such user"); + } $e = send_event(new UserPageBuildingEvent($display_user)); $this->display_stats($e); - } else { + } catch (UserNotFound $ex) { $this->theme->display_error( 404, "No Such User", @@ -519,8 +521,11 @@ class UserPage extends Extension "letters, numbers, dash, and underscore" ); } - if (User::by_name($name)) { + try { + User::by_name($name); throw new UserCreationException("That username is already taken"); + } catch (UserNotFound $ex) { + // user not found is good } if (!captcha_check()) { throw new UserCreationException("Error in captcha"); @@ -588,11 +593,6 @@ class UserPage extends Extension $matches = []; if (preg_match(self::USER_SEARCH_REGEX, $event->term, $matches)) { $duser = User::by_name($matches[2]); - if (is_null($duser)) { - throw new SearchTermParseException( - "Can't find the user named " . html_escape($matches[2]) - ); - } $event->add_querylet(new Querylet("images.owner_id {$matches[1]}= {$duser->id}")); } elseif (preg_match(self::USER_ID_SEARCH_REGEX, $event->term, $matches)) { $user_id = int_escape($matches[2]); @@ -630,19 +630,15 @@ class UserPage extends Extension global $config, $page; $duser = User::by_name_and_pass($name, $pass); - if (!is_null($duser)) { - send_event(new UserLoginEvent($duser)); - $duser->set_login_cookie(); - $page->set_mode(PageMode::REDIRECT); + send_event(new UserLoginEvent($duser)); + $duser->set_login_cookie(); + $page->set_mode(PageMode::REDIRECT); - // Try returning to previous page - if ($config->get_int("user_loginshowprofile", 0)) { - $page->set_redirect(referer_or(make_link(), ["user/"])); - } else { - $page->set_redirect(make_link("user")); - } + // Try returning to previous page + if ($config->get_int("user_loginshowprofile", 0)) { + $page->set_redirect(referer_or(make_link(), ["user/"])); } else { - $this->theme->display_error(401, "Error", "No user with those details was found"); + $page->set_redirect(make_link("user")); } } @@ -669,9 +665,7 @@ class UserPage extends Extension private function page_recover(string $username): void { $my_user = User::by_name($username); - if (is_null($my_user)) { - $this->theme->display_error(404, "Error", "There's no user with that name"); - } elseif (is_null($my_user->email)) { + if (is_null($my_user->email)) { $this->theme->display_error(400, "Error", "That user has no registered email address"); } else { throw new ServerError("Email sending not implemented"); diff --git a/ext/user/test.php b/ext/user/test.php index be275669..d398cf51 100644 --- a/ext/user/test.php +++ b/ext/user/test.php @@ -66,7 +66,7 @@ class UserPageTest extends ShimmiePHPUnitTestCase 'email' => '', ]); }); - $this->assertNull(User::by_name('testnew'), "Anon can't create others"); + $this->assertException(UserNotFound::class, function () {User::by_name('testnew');}); $this->log_in_as_admin(); $this->post_page('user_admin/create_other', [ @@ -76,6 +76,6 @@ class UserPageTest extends ShimmiePHPUnitTestCase 'email' => '', ]); $this->assertEquals(302, $page->code); - $this->assertNotNull(User::by_name('testnew'), "Admin can create others"); + $this->assertNotNull(User::by_name('testnew')); } } diff --git a/ext/user_config/main.php b/ext/user_config/main.php index 1b696884..537fd9ce 100644 --- a/ext/user_config/main.php +++ b/ext/user_config/main.php @@ -66,7 +66,6 @@ class UserConfig extends Extension global $database; $user = User::by_id($id); - $user_config = new DatabaseConfig($database, "user_config", "user_id", "$id"); send_event(new InitUserConfigEvent($user, $user_config)); return $user_config;