Skip to content

Commit 5576bb8

Browse files
RichardAndersonRichardAnderson
authored
[Fix] Cron job tooling (#1123)
1 parent afc0678 commit 5576bb8

7 files changed

Lines changed: 103 additions & 17 deletions

File tree

‎app/Actions/CronJob/SyncCronJobs.php‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ private function syncUserCronJobs(Server $server, string $user): void
9999

100100
// Check if this matches any Vito-managed cronjob (including site-level ones)
101101
$matchingCronJob = $vitoCronJobs->first(function ($cronJob) use ($frequency, $command) {
102-
return $this->normalizeFrequency($cronJob->frequency) === $frequency && $this->normalizeCommand($cronJob->command) === $command;
102+
return $this->normalizeFrequency($cronJob->frequency) === $frequency && $this->matchesCommand($cronJob->command, $command);
103103
});
104104

105105
if ($matchingCronJob) {
@@ -126,7 +126,7 @@ private function syncUserCronJobs(Server $server, string $user): void
126126
// Create new cronjobs for manually created ones (not in Vito)
127127
foreach ($serverCronJobs as $cronJobData) {
128128
$isVitoManaged = $vitoCronJobs->contains(function ($cronJob) use ($cronJobData) {
129-
return $this->normalizeFrequency($cronJob->frequency) === $cronJobData['frequency'] && $this->normalizeCommand($cronJob->command) === $cronJobData['command'];
129+
return $this->normalizeFrequency($cronJob->frequency) === $cronJobData['frequency'] && $this->matchesCommand($cronJob->command, $cronJobData['command']);
130130
});
131131

132132
if (! $isVitoManaged) {
@@ -154,6 +154,18 @@ private function normalizeCommand(string $command): string
154154
return preg_replace('/\s+/', ' ', trim($command));
155155
}
156156

157+
private function matchesCommand(string $cronJobCommand, string $serverCommand): bool
158+
{
159+
$normalizedCronJob = $this->normalizeCommand($cronJobCommand);
160+
if ($normalizedCronJob === $serverCommand) {
161+
return true;
162+
}
163+
164+
$unwrapped = CronJob::unwrapCommand($serverCommand);
165+
166+
return $unwrapped !== null && $this->normalizeCommand($unwrapped) === $normalizedCronJob;
167+
}
168+
157169
/**
158170
* @throws SSHError
159171
*/

‎app/Http/Controllers/CronJobController.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ public function site(Server $server, Site $site): Response
120120
),
121121
'site' => $site,
122122
'sites' => $server->sites()->select('id', 'domain')->get(),
123+
'ssh_users' => $site->getSshUsers(),
123124
]);
124125
}
125126

‎app/Models/CronJob.php‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,8 @@ public static function crontab(Server $server, string $user): string
8484
->get();
8585
/** @var CronJob $cronJob */
8686
foreach ($cronJobs as $key => $cronJob) {
87-
$data .= $cronJob->frequency.' '.$cronJob->command;
87+
$command = $user === 'root' ? $cronJob->command : self::wrapCommand($cronJob->command);
88+
$data .= $cronJob->frequency.' '.$command;
8889
if ($key != count($cronJobs) - 1) {
8990
$data .= "\n";
9091
}
@@ -93,6 +94,26 @@ public static function crontab(Server $server, string $user): string
9394
return $data;
9495
}
9596

97+
public static function wrapCommand(string $command): string
98+
{
99+
return 'bash -lc '.escapeshellarg($command);
100+
}
101+
102+
public static function unwrapCommand(string $command): ?string
103+
{
104+
$prefix = 'bash -lc ';
105+
if (! str_starts_with($command, $prefix)) {
106+
return null;
107+
}
108+
109+
$argument = trim(substr($command, strlen($prefix)));
110+
if (strlen($argument) < 2 || ! str_starts_with($argument, "'") || ! str_ends_with($argument, "'")) {
111+
return null;
112+
}
113+
114+
return str_replace("'\\''", "'", substr($argument, 1, -1));
115+
}
116+
96117
public function frequencyLabel(): string
97118
{
98119
$labels = [

‎app/SSH/OS/Cron.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ public function update(string $user, string $cron): void
1616
{
1717
$this->server->ssh()->exec(
1818
view('ssh.cron.update', [
19-
'cron' => $cron,
19+
'cron' => str_replace("'", "'\\''", $cron),
2020
'user' => $user,
2121
]),
2222
'update-cron'

‎resources/js/pages/cronjobs/components/form.tsx‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import {
88
DialogTitle,
99
DialogTrigger,
1010
} from '@/components/ui/dialog';
11-
import React, { FormEvent, ReactNode, useState } from 'react';
11+
import React, { FormEvent, ReactNode, useMemo, useState } from 'react';
1212
import { Form, FormField, FormFields } from '@/components/ui/form';
1313
import { Button } from '@/components/ui/button';
1414
import { useForm, usePage } from '@inertiajs/react';
@@ -34,9 +34,17 @@ export default function CronJobForm({
3434
cronJob?: CronJob;
3535
children: ReactNode;
3636
}) {
37-
const page = usePage<SharedData & { server: Server; sites?: Array<{ id: number; domain: string }> }>();
37+
const page = usePage<SharedData & { server: Server; sites?: Array<{ id: number; domain: string }>; ssh_users?: string[] }>();
3838
const configs = useConfigs()!;
3939
const [open, setOpen] = useState(false);
40+
41+
const sshUsers = useMemo(() => {
42+
const base = site ? (page.props.ssh_users ?? []) : page.props.server.ssh_users;
43+
if (cronJob?.user && !base.includes(cronJob.user)) {
44+
return [...base, cronJob.user];
45+
}
46+
return base;
47+
}, [site, page.props.ssh_users, page.props.server.ssh_users, cronJob?.user]);
4048
const form = useForm<{
4149
name: string;
4250
command: string;
@@ -47,7 +55,7 @@ export default function CronJobForm({
4755
}>({
4856
name: cronJob?.name || '',
4957
command: cronJob?.command || '',
50-
user: cronJob?.user || '',
58+
user: cronJob?.user || (site?.isolated_user_id ? site.user : ''),
5159
frequency: cronJob ? (configs.cronjob_intervals[cronJob.frequency] ? cronJob.frequency : 'custom') : '',
5260
custom: cronJob?.frequency || '',
5361
site_id: cronJob?.site_id?.toString() || '0',
@@ -175,7 +183,7 @@ export default function CronJobForm({
175183
</SelectTrigger>
176184
<SelectContent>
177185
<SelectGroup>
178-
{page.props.server.ssh_users.map((user) => (
186+
{sshUsers.map((user) => (
179187
<SelectItem key={`user-${user}`} value={user}>
180188
{user}
181189
</SelectItem>

‎tests/Feature/CronjobTest.php‎

Lines changed: 48 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace Tests\Feature;
44

5+
use App\Actions\CronJob\SyncCronJobs;
56
use App\Enums\CronjobStatus;
67
use App\Facades\SSH;
78
use App\Models\CronJob;
@@ -76,7 +77,7 @@ public function test_create_cronjob(): void
7677
'name' => 'My Cronjob',
7778
]);
7879

79-
SSH::assertExecutedContains("echo '* * * * * ls -la' | sudo -u vito crontab -");
80+
SSH::assertExecutedContains("echo '* * * * * bash -lc '\\''ls -la'\\''' | sudo -u vito crontab -");
8081
SSH::assertExecutedContains('sudo -u vito crontab -l');
8182
}
8283

@@ -100,7 +101,7 @@ public function test_create_cronjob_for_isolated_user(): void
100101
'user' => 'example',
101102
]);
102103

103-
SSH::assertExecutedContains("echo '* * * * * ls -la' | sudo -u example crontab -");
104+
SSH::assertExecutedContains("echo '* * * * * bash -lc '\\''ls -la'\\''' | sudo -u example crontab -");
104105
SSH::assertExecutedContains('sudo -u example crontab -l');
105106
}
106107

@@ -166,10 +167,53 @@ public function test_create_custom_cronjob(): void
166167
'status' => CronjobStatus::READY,
167168
]);
168169

169-
SSH::assertExecutedContains("echo '* * * 1 1 ls -la' | sudo -u vito crontab -");
170+
SSH::assertExecutedContains("echo '* * * 1 1 bash -lc '\\''ls -la'\\''' | sudo -u vito crontab -");
170171
SSH::assertExecutedContains('sudo -u vito crontab -l');
171172
}
172173

174+
public function test_cronjob_is_wrapped_in_login_shell_and_survives_sync(): void
175+
{
176+
SSH::fake();
177+
$this->actingAs($this->user);
178+
179+
/** @var CronJob $cronjob */
180+
$cronjob = CronJob::factory()->create([
181+
'server_id' => $this->server->id,
182+
'user' => 'vito',
183+
'command' => "echo 'hi'",
184+
'frequency' => '* * * * *',
185+
'status' => CronjobStatus::READY,
186+
'site_id' => null,
187+
]);
188+
189+
$crontab = CronJob::crontab($this->server, 'vito');
190+
$this->assertSame("* * * * * bash -lc 'echo '\\''hi'\\'''", $crontab);
191+
$this->assertSame("echo 'hi'", CronJob::unwrapCommand("bash -lc 'echo '\\''hi'\\'''"));
192+
193+
SSH::fake($crontab);
194+
app(SyncCronJobs::class)->sync($this->server);
195+
196+
$cronjob->refresh();
197+
$this->assertSame(CronjobStatus::READY, $cronjob->status);
198+
$this->assertSame(1, CronJob::query()->where('user', 'vito')->where('command', "echo 'hi'")->count());
199+
}
200+
201+
public function test_root_cronjob_is_not_wrapped(): void
202+
{
203+
$this->actingAs($this->user);
204+
205+
CronJob::factory()->create([
206+
'server_id' => $this->server->id,
207+
'user' => 'root',
208+
'command' => '/usr/bin/backup.sh',
209+
'frequency' => '0 2 * * *',
210+
'status' => CronjobStatus::READY,
211+
'site_id' => null,
212+
]);
213+
214+
$this->assertSame('0 2 * * * /usr/bin/backup.sh', CronJob::crontab($this->server, 'root'));
215+
}
216+
173217
public function test_enable_cronjob(): void
174218
{
175219
SSH::fake();
@@ -195,7 +239,7 @@ public function test_enable_cronjob(): void
195239

196240
$this->assertEquals(CronjobStatus::READY, $cronjob->status);
197241

198-
SSH::assertExecutedContains("echo '* * * 1 1 ls -la' | sudo -u vito crontab -");
242+
SSH::assertExecutedContains("echo '* * * 1 1 bash -lc '\\''ls -la'\\''' | sudo -u vito crontab -");
199243
SSH::assertExecutedContains('sudo -u vito crontab -l');
200244
}
201245

‎tests/Feature/SiteCronjobTest.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ public function test_create_site_cronjob(): void
8282
'status' => CronjobStatus::READY,
8383
]);
8484

85-
SSH::assertExecutedContains("echo '* * * * * ls -la' | sudo -u vito crontab -");
85+
SSH::assertExecutedContains("echo '* * * * * bash -lc '\\''ls -la'\\''' | sudo -u vito crontab -");
8686
SSH::assertExecutedContains('sudo -u vito crontab -l');
8787
}
8888

@@ -110,7 +110,7 @@ public function test_create_site_cronjob_for_isolated_user(): void
110110
'user' => 'example',
111111
]);
112112

113-
SSH::assertExecutedContains("echo '* * * * * ls -la' | sudo -u example crontab -");
113+
SSH::assertExecutedContains("echo '* * * * * bash -lc '\\''ls -la'\\''' | sudo -u example crontab -");
114114
SSH::assertExecutedContains('sudo -u example crontab -l');
115115
}
116116

@@ -162,7 +162,7 @@ public function test_create_custom_site_cronjob(): void
162162
'status' => CronjobStatus::READY,
163163
]);
164164

165-
SSH::assertExecutedContains("echo '* * * 1 1 ls -la' | sudo -u vito crontab -");
165+
SSH::assertExecutedContains("echo '* * * 1 1 bash -lc '\\''ls -la'\\''' | sudo -u vito crontab -");
166166
SSH::assertExecutedContains('sudo -u vito crontab -l');
167167
}
168168

@@ -193,7 +193,7 @@ public function test_enable_site_cronjob(): void
193193

194194
$this->assertEquals(CronjobStatus::READY, $cronjob->status);
195195

196-
SSH::assertExecutedContains("echo '* * * 1 1 ls -la' | sudo -u vito crontab -");
196+
SSH::assertExecutedContains("echo '* * * 1 1 bash -lc '\\''ls -la'\\''' | sudo -u vito crontab -");
197197
SSH::assertExecutedContains('sudo -u vito crontab -l');
198198
}
199199

@@ -260,7 +260,7 @@ public function test_update_site_cronjob(): void
260260
$this->assertEquals('php artisan schedule:run', $cronjob->command);
261261
$this->assertEquals('0 * * * *', $cronjob->frequency);
262262

263-
SSH::assertExecutedContains("echo '0 * * * * php artisan schedule:run' | sudo -u vito crontab -");
263+
SSH::assertExecutedContains("echo '0 * * * * bash -lc '\\''php artisan schedule:run'\\''' | sudo -u vito crontab -");
264264
SSH::assertExecutedContains('sudo -u vito crontab -l');
265265
}
266266
}

0 commit comments

Comments
 (0)