From dc71d87f6177a91051b98f767bb47af3136670b2 Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Sun, 12 Mar 2023 17:47:40 +0100 Subject: [PATCH] Make the RemoteUserProvider use a db persisted user & Update tests --- app/Api/v1/Controllers/UserController.php | 1 - app/Extensions/RemoteUserProvider.php | 95 +++++---- app/Services/Auth/ReverseProxyGuard.php | 11 +- resources/js/views/settings/Account.vue | 3 +- .../Extensions/RemoteUserProviderTest.php | 185 ++++++++++++++++++ .../AuthenticateMiddlewareTest.php | 39 ---- .../Extensions/RemoteUserProviderTest.php | 25 --- 7 files changed, 252 insertions(+), 107 deletions(-) create mode 100644 tests/Feature/Extensions/RemoteUserProviderTest.php delete mode 100644 tests/Unit/Extensions/RemoteUserProviderTest.php diff --git a/app/Api/v1/Controllers/UserController.php b/app/Api/v1/Controllers/UserController.php index 7c427682..ffd5d7f5 100644 --- a/app/Api/v1/Controllers/UserController.php +++ b/app/Api/v1/Controllers/UserController.php @@ -5,7 +5,6 @@ use App\Api\v1\Requests\SettingUpdateRequest; use App\Api\v1\Resources\UserResource; use App\Http\Controllers\Controller; -use App\Models\User; use Illuminate\Http\Request; use Illuminate\Support\Arr; diff --git a/app/Extensions/RemoteUserProvider.php b/app/Extensions/RemoteUserProvider.php index 9a8b1ab4..0fefe3da 100644 --- a/app/Extensions/RemoteUserProvider.php +++ b/app/Extensions/RemoteUserProvider.php @@ -9,20 +9,13 @@ use Exception; use Illuminate\Contracts\Auth\Authenticatable; use Illuminate\Contracts\Auth\UserProvider; -use Illuminate\Support\Arr; +use Illuminate\Support\Facades\Log; +use Illuminate\Support\Facades\Validator; +use Illuminate\Support\Str; +use Illuminate\Validation\ValidationException; class RemoteUserProvider implements UserProvider { - // 2FAuth is single user by design and domain data are not coupled to the user model. - // So the RemoteUserProvider provides a non-persisted user, dynamically instanciated using data - // from the auth proxy. - // - // This way no matter the user data set at proxy level, 2FAuth will always - // authenticate a request from the proxy and will return domain data without restriction. - // - // The downside of this approach is that we have to be sure that no change that needs - // to be persisted will be made to the user instance afterward (i.e through middlewares). - /** * The currently authenticated user. * @@ -30,39 +23,71 @@ class RemoteUserProvider implements UserProvider */ protected $user; - /** - * Get the In-memory user - * - * @return \App\Models\User - */ - protected function getInMemoryUser() - { - if (is_null($this->user)) { - $this->user = new User; - $this->user->name = 'Remote User'; - $this->user->email = 'fake.email@do.not.use'; - } - - return $this->user; - } - /** * {@inheritDoc} */ public function retrieveById($identifier) { - $user = $this->getInMemoryUser(); + // We don't know the id length so we trim it to prevent to long strings in DB + $name = substr($identifier['id'], 0, 180); + $email = null; - if (Arr::has($identifier, 'user')) { - $user->name = $identifier['user']; + $user = User::where('name', $name)->first(); + + // We use the passed email only if it is valid and no account is using it. + if ($identifier['email']) { + try { + $validated = Validator::validate([ + 'email' => $identifier['email'], + ], [ + 'email' => 'email', + ]); + + if (User::where('id', '<>', $user->id ?? 0)->where('email', $identifier['email'])->count() == 0) { + $email = $identifier['email']; + } + } catch (ValidationException $e) { + // do nothing + } } - if (Arr::has($identifier, 'email')) { - $user->email = $identifier['email']; + + $email = $email ?? $this->remoteEmail((string) $identifier['id']); + + if (is_null($user)) { + $user = User::create([ + 'name' => $name, + 'email' => strtolower($email), + 'password' => bcrypt(Str::random(64)), + ]); + + Log::info(sprintf('Remote user %s created with email address %s', var_export($user->name, true), var_export($user->email, true))); + + if (User::count() === 1) { + $user->is_admin = true; + $user->save(); + } + } else { + // Here we keep the account's email sync-ed + if ($user->email != $email) { + $user->email = $email; + $user->save(); + } } return $user; } + /** + * Set a fake email address + * + * @param $id string + * @return string + */ + protected function remoteEmail(string $id) + { + return substr($id, 0, 184) . '@remote'; + } + /** * {@inheritDoc} * @@ -70,7 +95,7 @@ public function retrieveById($identifier) */ public function retrieveByToken($identifier, $token) { - return $this->retrieveById($identifier); + throw new Exception(sprintf('No implementation for %s', __METHOD__)); } /** @@ -90,7 +115,7 @@ public function updateRememberToken(Authenticatable $user, $token) */ public function retrieveByCredentials(array $credentials) { - return $this->getInMemoryUser(); + throw new Exception(sprintf('No implementation for %s', __METHOD__)); } /** @@ -100,6 +125,6 @@ public function retrieveByCredentials(array $credentials) */ public function validateCredentials(Authenticatable $user, array $credentials) { - return true; + throw new Exception(sprintf('No implementation for %s', __METHOD__)); } } diff --git a/app/Services/Auth/ReverseProxyGuard.php b/app/Services/Auth/ReverseProxyGuard.php index 1f2dbab2..d49d7d04 100644 --- a/app/Services/Auth/ReverseProxyGuard.php +++ b/app/Services/Auth/ReverseProxyGuard.php @@ -50,19 +50,20 @@ public function user() $identifier = []; try { - $identifier['user'] = request()->server($remoteUserHeader) ?? apache_request_headers()[$remoteUserHeader] ?? null; + $identifier['id'] = request()->server($remoteUserHeader) ?? apache_request_headers()[$remoteUserHeader] ?? null; } catch (\Throwable $e) { - $identifier['user'] = null; + $identifier['id'] = null; } - if (! $identifier['user'] || is_array($identifier['user'])) { - Log::error(sprintf('Proxy remote-user header "%s" is empty or missing.', $remoteUserHeader)); + if (! $identifier['id'] || is_array($identifier['id'])) { + Log::error(sprintf('Proxy remote-user header %s is empty or missing.', var_export($remoteUserHeader, true))); return $this->user = null; } // Get the email identifier from $_SERVER - $remoteEmailHeader = config('auth.auth_proxy_headers.email'); + $remoteEmailHeader = config('auth.auth_proxy_headers.email'); + $identifier['email'] = null; if ($remoteEmailHeader) { try { diff --git a/resources/js/views/settings/Account.vue b/resources/js/views/settings/Account.vue index 95025493..bf01086e 100644 --- a/resources/js/views/settings/Account.vue +++ b/resources/js/views/settings/Account.vue @@ -68,7 +68,7 @@ formDelete: new Form({ password : '', }), - isRemoteUser: false, + isRemoteUser: this.$root.appConfig.proxyAuth, isAdmin: false, } }, @@ -77,7 +77,6 @@ const { data } = await this.formProfile.get('/api/v1/user') if( data.is_admin === true ) this.isAdmin = true - if( data.id === null ) this.isRemoteUser = true this.formProfile.fill(data) }, diff --git a/tests/Feature/Extensions/RemoteUserProviderTest.php b/tests/Feature/Extensions/RemoteUserProviderTest.php new file mode 100644 index 00000000..820f0c83 --- /dev/null +++ b/tests/Feature/Extensions/RemoteUserProviderTest.php @@ -0,0 +1,185 @@ +create([ + 'name' => self::USER_NAME, + 'email' => self::USER_EMAIL, + ]); + + $provider = new RemoteUserProvider; + + $user = $provider->retrieveById([ + 'id' => self::USER_NAME, + 'email' => null, + ]); + + $this->assertEquals($dbUser->id, $user->id); + } + + /** + * @test + */ + public function test_user_is_set_from_reverse_proxy_info_with_provided_email() + { + Config::set('auth.auth_proxy_headers.user', 'HTTP_REMOTE_USER'); + Config::set('auth.auth_proxy_headers.email', 'HTTP_REMOTE_EMAIL'); + + $this->app['auth']->shouldUse('reverse-proxy-guard'); + + $this->json('GET', '/api/v1/groups', [], [ + 'HTTP_REMOTE_USER' => self::USER_NAME, + 'HTTP_REMOTE_EMAIL' => self::USER_EMAIL, + ]); + $this->assertAuthenticated('reverse-proxy-guard'); + + $user = $this->app->make('auth')->guard('reverse-proxy-guard')->user(); + $this->assertEquals(self::USER_NAME, $user->name); + $this->assertEquals(self::USER_EMAIL, $user->email); + } + + /** + * @test + */ + public function test_user_is_set_from_reverse_proxy_without_email() + { + Config::set('auth.auth_proxy_headers.user', 'HTTP_REMOTE_USER'); + + $this->app['auth']->shouldUse('reverse-proxy-guard'); + + $this->json('GET', '/api/v1/groups', [], [ + 'HTTP_REMOTE_USER' => self::USER_NAME, + ]); + $this->assertAuthenticated('reverse-proxy-guard'); + + $user = $this->app->make('auth')->guard('reverse-proxy-guard')->user(); + $this->assertEquals(self::USER_NAME, $user->name); + $this->assertEquals(strtolower(self::USER_NAME) . '@remote', $user->email); + } + + /** + * @test + */ + public function test_user_is_set_from_reverse_proxy_even_if_identifier_is_long() + { + Config::set('auth.auth_proxy_headers.user', 'HTTP_REMOTE_USER'); + Config::set('auth.auth_proxy_headers.email', 'HTTP_REMOTE_EMAIL'); + + $this->app['auth']->shouldUse('reverse-proxy-guard'); + + $name = str_pad('john', 300, '_'); + + $this->json('GET', '/api/v1/groups', [], [ + 'HTTP_REMOTE_USER' => $name, + ]); + $this->assertAuthenticated('reverse-proxy-guard'); + + $user = $this->app->make('auth')->guard('reverse-proxy-guard')->user(); + $this->assertLessThan(192, strlen($user->name)); + $this->assertLessThan(192, strlen($user->email)); + } + + /** + * @test + */ + public function test_user_email_is_sync_with_email_proxy_header() + { + Config::set('auth.auth_proxy_headers.user', 'HTTP_REMOTE_USER'); + Config::set('auth.auth_proxy_headers.email', 'HTTP_REMOTE_EMAIL'); + + $dbUser = User::factory()->create([ + 'name' => self::USER_NAME, + 'email' => strtolower(self::USER_NAME) . '@remote', + ]); + + $this->app['auth']->shouldUse('reverse-proxy-guard'); + + $this->json('GET', '/api/v1/groups', [], [ + 'HTTP_REMOTE_USER' => self::USER_NAME, + 'HTTP_REMOTE_EMAIL' => self::USER_EMAIL, + ]); + + $this->assertAuthenticated('reverse-proxy-guard'); + + $this->assertDatabaseHas('users', [ + 'id' => $dbUser->id, + 'email' => self::USER_EMAIL, + ]); + } + + /** + * @test + */ + public function test_user_email_is_not_sync_with_invalid_email_proxy_header() + { + Config::set('auth.auth_proxy_headers.user', 'HTTP_REMOTE_USER'); + Config::set('auth.auth_proxy_headers.email', 'HTTP_REMOTE_EMAIL'); + + $dbUser = User::factory()->create([ + 'name' => self::USER_NAME, + 'email' => strtolower(self::USER_NAME) . '@remote', + ]); + + $this->app['auth']->shouldUse('reverse-proxy-guard'); + + $this->json('GET', '/api/v1/groups', [], [ + 'HTTP_REMOTE_USER' => self::USER_NAME, + 'HTTP_REMOTE_EMAIL' => 'bad[at]email', + ]); + + $this->assertAuthenticated('reverse-proxy-guard'); + + $this->assertDatabaseHas('users', [ + 'id' => $dbUser->id, + 'email' => $dbUser->email, + ]); + } + + /** + * @test + */ + public function test_user_email_is_not_sync_with_already_used_email_proxy_header() + { + Config::set('auth.auth_proxy_headers.user', 'HTTP_REMOTE_USER'); + Config::set('auth.auth_proxy_headers.email', 'HTTP_REMOTE_EMAIL'); + + $dbUser = User::factory()->create([ + 'name' => self::USER_NAME, + 'email' => strtolower(self::USER_NAME) . '@remote', + ]); + + $otherUser = User::factory()->create([ + 'email' => 'other@example.com', + ]); + + $this->app['auth']->shouldUse('reverse-proxy-guard'); + + $this->json('GET', '/api/v1/groups', [], [ + 'HTTP_REMOTE_USER' => self::USER_NAME, + 'HTTP_REMOTE_EMAIL' => $otherUser->email, + ]); + + $this->assertAuthenticated('reverse-proxy-guard'); + + $this->assertDatabaseHas('users', [ + 'id' => $dbUser->id, + 'email' => $dbUser->email, + ]); + } +} diff --git a/tests/Feature/Http/Middlewares/AuthenticateMiddlewareTest.php b/tests/Feature/Http/Middlewares/AuthenticateMiddlewareTest.php index def57ec3..38a12f5d 100644 --- a/tests/Feature/Http/Middlewares/AuthenticateMiddlewareTest.php +++ b/tests/Feature/Http/Middlewares/AuthenticateMiddlewareTest.php @@ -24,45 +24,6 @@ public function test_it_always_authenticates_with_reverse_proxy_guard() $this->assertAuthenticated('reverse-proxy-guard'); } - /** - * @test - */ - public function test_user_is_set_from_reverse_proxy_info() - { - Config::set('auth.auth_proxy_headers.user', 'HTTP_REMOTE_USER'); - Config::set('auth.auth_proxy_headers.email', 'HTTP_REMOTE_EMAIL'); - - $this->app['auth']->shouldUse('reverse-proxy-guard'); - - $this->json('GET', '/api/v1/groups', [], [ - 'HTTP_REMOTE_USER' => self::USER_NAME, - 'HTTP_REMOTE_EMAIL' => self::USER_EMAIL, - ]); - $this->assertAuthenticated('reverse-proxy-guard'); - - $user = $this->app->make('auth')->guard('reverse-proxy-guard')->user(); - $this->assertEquals(self::USER_NAME, $user->name); - $this->assertEquals(self::USER_EMAIL, $user->email); - } - - /** - * @test - */ - public function test_user_is_set_from_reverse_proxy_without_email() - { - Config::set('auth.auth_proxy_headers.user', 'HTTP_REMOTE_USER'); - - $this->app['auth']->shouldUse('reverse-proxy-guard'); - - $this->json('GET', '/api/v1/groups', [], [ - 'HTTP_REMOTE_USER' => self::USER_NAME, - ]); - $this->assertAuthenticated('reverse-proxy-guard'); - - $user = $this->app->make('auth')->guard('reverse-proxy-guard')->user(); - $this->assertEquals('fake.email@do.not.use', $user->email); - } - /** * @test */ diff --git a/tests/Unit/Extensions/RemoteUserProviderTest.php b/tests/Unit/Extensions/RemoteUserProviderTest.php deleted file mode 100644 index 171436f3..00000000 --- a/tests/Unit/Extensions/RemoteUserProviderTest.php +++ /dev/null @@ -1,25 +0,0 @@ -retrieveById([ - 'user' => 'testUser', - 'email' => 'test@example.org', - ]); - - $this->assertInstanceOf('\App\Models\User', $user); - $this->assertEquals(true, $user->exists); - } -}