Skip to content

Commit 4b4d8ba

Browse files
ssddanbrownssddanbrown
committed
Avatar Commend: Simplified and updated during review
During review of #4560. - Simplified command to share as much log as possible across different run options. - Extracted out user handling to share with MFA command. - Added specific handling for disabled avatar fetching. - Added mention of avatar endpoint, to make it clear where these avatars are coming from (Protect against user expectation of LDAP avatar sync). - Simplified a range of the testing. - Tweaked wording and code formatting.
1 parent ca98155 commit 4b4d8ba

5 files changed

Lines changed: 183 additions & 266 deletions

File tree

‎app/Console/Commands/HandlesSingleUser.php‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<?php
2+
3+
namespace BookStack\Console\Commands;
4+
5+
use BookStack\Users\Models\User;
6+
use Exception;
7+
use Illuminate\Console\Command;
8+
9+
/**
10+
* @mixin Command
11+
*/
12+
trait HandlesSingleUser
13+
{
14+
/**
15+
* Fetch a user provided to this command.
16+
* Expects the command to accept 'id' and 'email' options.
17+
* @throws Exception
18+
*/
19+
private function fetchProvidedUser(): User
20+
{
21+
$id = $this->option('id');
22+
$email = $this->option('email');
23+
if (!$id && !$email) {
24+
throw new Exception("Either a --id=<number> or --email=<email> option must be provided.\nRun this command with `--help` to show more options.");
25+
}
26+
27+
$field = $id ? 'id' : 'email';
28+
$value = $id ?: $email;
29+
30+
$user = User::query()
31+
->where($field, '=', $value)
32+
->first();
33+
34+
if (!$user) {
35+
throw new Exception("A user where {$field}={$value} could not be found.");
36+
}
37+
38+
return $user;
39+
}
40+
}

‎app/Console/Commands/RefreshAvatarCommand.php‎

Lines changed: 39 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
11
<?php
22

3-
declare(strict_types=1);
4-
53
namespace BookStack\Console\Commands;
64

75
use BookStack\Users\Models\User;
6+
use Exception;
87
use Illuminate\Console\Command;
98
use BookStack\Uploads\UserAvatars;
10-
use Illuminate\Database\Eloquent\Collection;
119

12-
final class RefreshAvatarCommand extends Command
10+
class RefreshAvatarCommand extends Command
1311
{
12+
use HandlesSingleUser;
13+
1414
/**
1515
* The name and signature of the console command.
1616
*
@@ -20,128 +20,89 @@ final class RefreshAvatarCommand extends Command
2020
{--id= : Numeric ID of the user to refresh avatar for}
2121
{--email= : Email address of the user to refresh avatar for}
2222
{--users-without-avatars : Refresh avatars for users that currently have no avatar}
23-
{--a|all : Refresh all user avatars}
24-
{--f|force : Actually run the update for --users-without-avatars, Defaults to a dry-run}';
23+
{--a|all : Refresh avatars for all users}
24+
{--f|force : Actually run the update, Defaults to a dry-run}';
2525

2626
/**
2727
* The console command description.
2828
*
2929
* @var string
3030
*/
31-
protected $description = 'Refresh avatar for given user or users';
31+
protected $description = 'Refresh avatar for the given user(s)';
3232

3333
public function handle(UserAvatars $userAvatar): int
3434
{
35-
$dryRun = !$this->option('force');
35+
if (!$userAvatar->avatarFetchEnabled()) {
36+
$this->error("Avatar fetching is disabled on this instance.");
37+
return self::FAILURE;
38+
}
3639

3740
if ($this->option('users-without-avatars')) {
38-
return $this->handleUpdateWithoutAvatars($userAvatar, $dryRun);
41+
return $this->processUsers(User::query()->whereDoesntHave('avatar')->get()->all(), $userAvatar);
3942
}
4043

4144
if ($this->option('all')) {
42-
return $this->handleUpdateAllAvatars($userAvatar, $dryRun);
45+
return $this->processUsers(User::query()->get()->all(), $userAvatar);
4346
}
4447

45-
return $this->handleSingleUserUpdate($userAvatar);
48+
try {
49+
$user = $this->fetchProvidedUser();
50+
return $this->processUsers([$user], $userAvatar);
51+
} catch (Exception $exception) {
52+
$this->error($exception->getMessage());
53+
return self::FAILURE;
54+
}
4655
}
4756

48-
private function handleUpdateWithoutAvatars(UserAvatars $userAvatar, bool $dryRun): int
57+
/**
58+
* @param User[] $users
59+
*/
60+
private function processUsers(array $users, UserAvatars $userAvatar): int
4961
{
50-
$users = User::query()->where('image_id', '=', 0)->get();
51-
$this->info(count($users) . ' user(s) found without avatars.');
62+
$dryRun = !$this->option('force');
63+
$this->info(count($users) . " user(s) found to update avatars for.");
5264

53-
if (!$dryRun) {
54-
$proceed = !$this->input->isInteractive() || $this->confirm('Are you sure you want to refresh avatars of users that do not have one?');
55-
if (!$proceed) {
56-
return self::SUCCESS;
57-
}
65+
if (count($users) === 0) {
66+
return self::SUCCESS;
5867
}
5968

60-
return $this->processUsers($users, $userAvatar, $dryRun);
61-
}
62-
63-
private function handleUpdateAllAvatars(UserAvatars $userAvatar, bool $dryRun): int
64-
{
65-
$users = User::query()->get();
66-
$this->info(count($users) . ' user(s) found.');
67-
6869
if (!$dryRun) {
69-
$proceed = !$this->input->isInteractive() || $this->confirm('Are you sure you want to refresh avatars for ALL USERS?');
70+
$fetchHost = parse_url($userAvatar->getAvatarUrl(), PHP_URL_HOST);
71+
$this->warn("This will destroy any existing avatar images these users have, and attempt to fetch new avatar images from {$fetchHost}.");
72+
$proceed = !$this->input->isInteractive() || $this->confirm('Are you sure you want to proceed?');
7073
if (!$proceed) {
7174
return self::SUCCESS;
7275
}
7376
}
7477

75-
return $this->processUsers($users, $userAvatar, $dryRun);
76-
}
78+
$this->info("");
7779

78-
private function processUsers(Collection $users, UserAvatars $userAvatar, bool $dryRun): int
79-
{
8080
$exitCode = self::SUCCESS;
8181
foreach ($users as $user) {
82-
$this->getOutput()->write("ID {$user->id} - ", false);
82+
$linePrefix = "[ID: {$user->id}] $user->email -";
8383

8484
if ($dryRun) {
85-
$this->warn('Not updated');
85+
$this->warn("{$linePrefix} Not updated");
8686
continue;
8787
}
8888

8989
if ($this->fetchAvatar($userAvatar, $user)) {
90-
$this->info('Updated');
90+
$this->info("{$linePrefix} Updated");
9191
} else {
92-
$this->error('Not updated');
92+
$this->error("{$linePrefix} Not updated");
9393
$exitCode = self::FAILURE;
9494
}
9595
}
9696

97-
$this->getOutput()->newLine();
9897
if ($dryRun) {
99-
$this->comment('Dry run, no avatars have been updated');
100-
$this->comment('Run with -f or --force to perform the update');
98+
$this->comment("");
99+
$this->comment("Dry run, no avatars were updated.");
100+
$this->comment('Run with -f or --force to perform the update.');
101101
}
102102

103103
return $exitCode;
104104
}
105105

106-
107-
private function handleSingleUserUpdate(UserAvatars $userAvatar): int
108-
{
109-
$id = $this->option('id');
110-
$email = $this->option('email');
111-
if (!$id && !$email) {
112-
$this->error('Either a --id=<number> or --email=<email> option must be provided.');
113-
$this->error('Run with `--help` to more options');
114-
115-
return self::FAILURE;
116-
}
117-
118-
$field = $id ? 'id' : 'email';
119-
$value = $id ?: $email;
120-
121-
$user = User::query()
122-
->where($field, '=', $value)
123-
->first();
124-
125-
if (!$user) {
126-
$this->error("A user where {$field}={$value} could not be found.");
127-
128-
return self::FAILURE;
129-
}
130-
131-
$this->info("This will refresh the avatar for user: \n- ID: {$user->id}\n- Name: {$user->name}\n- Email: {$user->email}\n");
132-
$confirm = $this->confirm('Are you sure you want to proceed?');
133-
if ($confirm) {
134-
if ($this->fetchAvatar($userAvatar, $user)) {
135-
$this->info('User avatar has been updated.');
136-
return self::SUCCESS;
137-
}
138-
139-
$this->info('Could not update avatar please review logs.');
140-
}
141-
142-
return self::FAILURE;
143-
}
144-
145106
private function fetchAvatar(UserAvatars $userAvatar, User $user): bool
146107
{
147108
$oldId = $user->avatar->id ?? 0;

‎app/Console/Commands/ResetMfaCommand.php‎

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@
22

33
namespace BookStack\Console\Commands;
44

5-
use BookStack\Users\Models\User;
5+
use Exception;
66
use Illuminate\Console\Command;
77

88
class ResetMfaCommand extends Command
99
{
10+
use HandlesSingleUser;
11+
1012
/**
1113
* The name and signature of the console command.
1214
*
@@ -29,25 +31,10 @@ class ResetMfaCommand extends Command
2931
*/
3032
public function handle(): int
3133
{
32-
$id = $this->option('id');
33-
$email = $this->option('email');
34-
if (!$id && !$email) {
35-
$this->error('Either a --id=<number> or --email=<email> option must be provided.');
36-
37-
return 1;
38-
}
39-
40-
$field = $id ? 'id' : 'email';
41-
$value = $id ?: $email;
42-
43-
/** @var User $user */
44-
$user = User::query()
45-
->where($field, '=', $value)
46-
->first();
47-
48-
if (!$user) {
49-
$this->error("A user where {$field}={$value} could not be found.");
50-
34+
try {
35+
$user = $this->fetchProvidedUser();
36+
} catch (Exception $exception) {
37+
$this->error($exception->getMessage());
5138
return 1;
5239
}
5340

‎app/Uploads/UserAvatars.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ protected function getAvatarImageData(string $url): string
127127
/**
128128
* Check if fetching external avatars is enabled.
129129
*/
130-
protected function avatarFetchEnabled(): bool
130+
public function avatarFetchEnabled(): bool
131131
{
132132
$fetchUrl = $this->getAvatarUrl();
133133

@@ -137,7 +137,7 @@ protected function avatarFetchEnabled(): bool
137137
/**
138138
* Get the URL to fetch avatars from.
139139
*/
140-
protected function getAvatarUrl(): string
140+
public function getAvatarUrl(): string
141141
{
142142
$configOption = config('services.avatar_url');
143143
if ($configOption === false) {

0 commit comments

Comments
 (0)