Skip to content

Commit 7075664

Browse files
committed
fix: Login view for challenge with missing user
1 parent 8f7a08c commit 7075664

2 files changed

Lines changed: 120 additions & 5 deletions

File tree

src/Panel/Controller/View/LoginViewController.php

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use Kirby\Auth\Pending;
88
use Kirby\Auth\State;
99
use Kirby\Auth\Status;
10+
use Kirby\Cms\User;
1011
use Kirby\Exception\LogicException;
1112
use Kirby\Panel\Controller\ViewController;
1213
use Kirby\Panel\Panel;
@@ -62,9 +63,8 @@ private function challenge(string|null $name): Challenge
6263
}
6364

6465
$challenges = $this->kirby->auth()->challenges();
65-
$email = $this->status->email();
66-
$user = $this->kirby->user($email);
6766
$mode = $this->status->mode();
67+
$user = $this->user();
6868

6969
return $challenges->get($type, $user, $mode);
7070
}
@@ -75,9 +75,8 @@ private function challenges(): array
7575
return [];
7676
}
7777

78-
$email = $this->status->email();
79-
$user = $this->kirby->user($email);
8078
$mode = $this->status->mode();
79+
$user = $this->user();
8180
$challenges = $this->kirby->auth()->challenges();
8281
$available = $challenges->available($user, $mode);
8382
$currentType = $this->status->challenge();
@@ -172,4 +171,22 @@ protected function value(): array
172171
{
173172
return [];
174173
}
174+
175+
/**
176+
* Returns the user the pending challenge belongs to.
177+
*
178+
* The challenge email may not belong to an existing user:
179+
* the account could have been deleted after the challenge
180+
* was created, or it never existed and the pending state
181+
* was only created to avoid leaking whether
182+
* the user exists (see `Auth::createChallenge()`).
183+
*
184+
* A virtual user lets the view render the form and
185+
* resolve challenge availability identically in that case.
186+
*/
187+
protected function user(): User
188+
{
189+
$email = $this->status->email();
190+
return $this->kirby->user($email) ?? new User(['email' => $email]);
191+
}
175192
}

tests/Panel/Controller/View/LoginViewControllerTest.php

Lines changed: 99 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,34 @@ public function render(): array|null
8989
}
9090
}
9191

92+
class TestSecretChallenge extends Challenge
93+
{
94+
// mirrors TotpChallenge: only available once the user has set it up
95+
public static function isAvailable(User $user, string $mode): bool
96+
{
97+
return $user->secret('totp') !== null;
98+
}
99+
100+
public function create(): Pending|null
101+
{
102+
return null;
103+
}
104+
105+
public function form(Pending $pending): Component
106+
{
107+
return new Component(
108+
component: 'k-login-test-secret-challenge-form',
109+
submit: $this->submit(),
110+
user: $this->user->email(),
111+
);
112+
}
113+
114+
public function verify(mixed $input, Pending $data): bool
115+
{
116+
return true;
117+
}
118+
}
119+
92120
#[CoversClass(LoginViewController::class)]
93121
class LoginViewControllerTest extends TestCase
94122
{
@@ -100,6 +128,7 @@ public function setUp(): void
100128
Challenges::$challenges['test'] = TestChallenge::class;
101129
Challenges::$challenges['test2'] = TestChallenge2::class;
102130
Challenges::$challenges['test-null-form'] = TestNullFormChallenge::class;
131+
Challenges::$challenges['test-secret'] = TestSecretChallenge::class;
103132
}
104133

105134
public function tearDown(): void
@@ -109,7 +138,8 @@ public function tearDown(): void
109138
unset(
110139
Challenges::$challenges['test'],
111140
Challenges::$challenges['test2'],
112-
Challenges::$challenges['test-null-form']
141+
Challenges::$challenges['test-null-form'],
142+
Challenges::$challenges['test-secret']
113143
);
114144
}
115145

@@ -329,6 +359,74 @@ public function testLoadWithChallenge(): void
329359
$this->assertFalse($props['challenges'][1]['active']);
330360
}
331361

362+
public function testLoadWithChallengeForNonExistentUser(): void
363+
{
364+
// A pending challenge whose email belongs to no user: either the
365+
// account was deleted after the challenge was created, or it never
366+
// existed and the pending state is only shown to avoid leaking
367+
// whether the user exists (see Auth::createChallenge()). The view
368+
// must still render (no TypeError) and, because both challenges are
369+
// available to any user, the list must look exactly like it does
370+
// for a real account (cf. testChallenges) so existence cannot leak.
371+
$this->app = $this->app->clone([
372+
'options' => [
373+
'auth' => [
374+
'challenges' => ['test', 'test2']
375+
]
376+
],
377+
'users' => [] // no users at all
378+
]);
379+
380+
$this->app->session()->set('kirby.challenge.email', 'ghost@example.com');
381+
$this->app->session()->set('kirby.challenge.type', 'test');
382+
$this->app->session()->set('kirby.challenge.mode', 'login');
383+
384+
$props = (new LoginViewController('challenge', 'test'))->load()->props();
385+
386+
$this->assertSame('pending', $props['state']);
387+
388+
// the form renders from a virtual user carrying the submitted email
389+
$this->assertSame('k-login-test-challenge-form', $props['form']['component']);
390+
$this->assertSame('ghost@example.com', $props['form']['props']['user']);
391+
392+
$this->assertCount(2, $props['challenges']);
393+
$this->assertSame('test', $props['challenges'][0]['type']);
394+
$this->assertTrue($props['challenges'][0]['active']);
395+
$this->assertSame('test2', $props['challenges'][1]['type']);
396+
$this->assertFalse($props['challenges'][1]['active']);
397+
}
398+
399+
public function testLoadExcludesUserSpecificChallengeForNonExistentUser(): void
400+
{
401+
// 'test-secret' is only available once the user set it up (like TOTP),
402+
// 'test2' is always available. For a non-existent user the virtual
403+
// user has no secret, so 'test-secret' is excluded just as it would
404+
// be for an existing account that never enabled it - leaving the
405+
// fallback challenge ('test2', the last enabled) as the only entry.
406+
$this->app = $this->app->clone([
407+
'options' => [
408+
'auth' => [
409+
'challenges' => ['test-secret', 'test2']
410+
]
411+
],
412+
'users' => []
413+
]);
414+
415+
$session = $this->app->session();
416+
$session->set('kirby.challenge.email', 'ghost@example.com');
417+
$session->set('kirby.challenge.mode', 'login');
418+
// note: no kirby.challenge.type, fallback to last enabled ('test2')
419+
420+
$props = (new LoginViewController('challenge', 'test2'))->load()->props();
421+
422+
$this->assertSame('k-login-test-challenge2-form', $props['form']['component']);
423+
$this->assertSame('ghost@example.com', $props['form']['props']['user']);
424+
425+
$types = array_column($props['challenges'], 'type');
426+
$this->assertSame(['test2'], $types);
427+
$this->assertNotContains('test-secret', $types);
428+
}
429+
332430
public function testLoadAppliesValueToForm(): void
333431
{
334432
// subclass that returns a value() so form props get prefilled

0 commit comments

Comments
 (0)