Allow multiple redirects when creating clients

This change allows for multiple redirects in a comma separated list when adding them through the ClientController. It makes use of a custom validation rule which will separate the urls by the comma and run the url validator rule over each one individually.

Except for https://example.com individually, the value of https://example.com,https://example2.com would be allowed as well.

I took the approach of a custom validation rule because it seemed the most flexible in terms of implementing the custom logic. It allows for re-using Laravel's url validation rule.

Fixes #897

Author: driesvints

src/Http/Controllers/ClientController.php

@@ -5,6 +5,7 @@
 use Illuminate\Http\Request;
 use Illuminate\Http\Response;
 use Laravel\Passport\ClientRepository;
+use Laravel\Passport\Http\Rules\RedirectRule;
 use Illuminate\Contracts\Validation\Factory as ValidationFactory;
 
 class ClientController
@@ -23,18 +24,29 @@ class ClientController
      */
     protected $validation;
 
+    /**
+     * The redirect validation rule.
+     *
+     * @var \Laravel\Passport\Http\Rules\RedirectRule
+     */
+    protected $redirectRule;
+
     /**
      * Create a client controller instance.
      *
      * @param  \Laravel\Passport\ClientRepository  $clients
      * @param  \Illuminate\Contracts\Validation\Factory  $validation
+     * @param  \Laravel\Passport\Http\Rules\RedirectRule  $redirectRule
      * @return void
      */
-    public function __construct(ClientRepository $clients,
-                                ValidationFactory $validation)
-    {
+    public function __construct(
+        ClientRepository $clients,
+        ValidationFactory $validation,
+        RedirectRule $redirectRule
+    ) {
         $this->clients = $clients;
         $this->validation = $validation;
+        $this->redirectRule = $redirectRule;
     }
 
     /**
@@ -60,7 +72,7 @@ public function store(Request $request)
     {
         $this->validation->make($request->all(), [
             'name' => 'required|max:255',
-            'redirect' => 'required|url',
+            'redirect' => ['required', $this->redirectRule],
         ])->validate();
 
         return $this->clients->create(
@@ -85,7 +97,7 @@ public function update(Request $request, $clientId)
 
         $this->validation->make($request->all(), [
             'name' => 'required|max:255',
-            'redirect' => 'required|url',
+            'redirect' => ['required', $this->redirectRule],
         ])->validate();
 
         return $this->clients->update(

src/Http/Rules/RedirectRule.php

@@ -0,0 +1,43 @@
+<?php
+
+namespace Laravel\Passport\Http\Rules;
+
+use Illuminate\Contracts\Validation\Rule;
+use Illuminate\Contracts\Validation\Factory;
+
+class RedirectRule implements Rule
+{
+    /**
+     * @var \Illuminate\Contracts\Validation\Factory
+     */
+    private $validator;
+
+    public function __construct(Factory $validator)
+    {
+        $this->validator = $validator;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function passes($attribute, $value)
+    {
+        foreach (explode(',', $value) as $redirect) {
+            $validator = $this->validator->make(['redirect' => $redirect], ['redirect' => 'url']);
+
+            if ($validator->fails()) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function message()
+    {
+        return 'One or more redirects have an invalid url format.';
+    }
+}

tests/ClientControllerTest.php

@@ -2,11 +2,12 @@
 
 namespace Laravel\Passport\Tests;
 
-use Laravel\Passport\Client;
-use Laravel\Passport\Http\Controllers\ClientController;
 use Mockery as m;
 use Illuminate\Http\Request;
+use Laravel\Passport\Client;
 use PHPUnit\Framework\TestCase;
+use Laravel\Passport\Http\Rules\RedirectRule;
+use Laravel\Passport\Http\Controllers\ClientController;
 
 class ClientControllerTest extends TestCase
 {
@@ -25,7 +26,9 @@ public function test_all_the_clients_for_the_current_user_can_be_retrieved()
         $request->shouldReceive('user')->andReturn(new ClientControllerFakeUser);
 
         $controller = new ClientController(
-            $clients, m::mock('Illuminate\Contracts\Validation\Factory')
+            $clients,
+            m::mock('Illuminate\Contracts\Validation\Factory'),
+            m::mock(RedirectRule::class)
         );
 
         $this->assertEquals($client, $controller->forUser($request));
@@ -45,17 +48,21 @@ public function test_clients_can_be_stored()
             ->with(1, 'client name', 'http://localhost')
             ->andReturn($client = new Client);
 
+        $redirectRule = m::mock(RedirectRule::class);
+
         $validator = m::mock('Illuminate\Contracts\Validation\Factory');
         $validator->shouldReceive('make')->once()->with([
             'name' => 'client name',
             'redirect' => 'http://localhost',
         ], [
             'name' => 'required|max:255',
-            'redirect' => 'required|url',
+            'redirect' => ['required', $redirectRule],
         ])->andReturn($validator);
         $validator->shouldReceive('validate')->once();
 
-        $controller = new ClientController($clients, $validator);
+        $controller = new ClientController(
+            $clients, $validator, $redirectRule
+        );
 
         $this->assertEquals($client, $controller->store($request));
     }
@@ -79,17 +86,21 @@ public function test_clients_can_be_updated()
             m::type('Laravel\Passport\Client'), 'client name', 'http://localhost'
         )->andReturn('response');
 
+        $redirectRule = m::mock(RedirectRule::class);
+
         $validator = m::mock('Illuminate\Contracts\Validation\Factory');
         $validator->shouldReceive('make')->once()->with([
             'name' => 'client name',
             'redirect' => 'http://localhost',
         ], [
             'name' => 'required|max:255',
-            'redirect' => 'required|url',
+            'redirect' => ['required', $redirectRule],
         ])->andReturn($validator);
         $validator->shouldReceive('validate')->once();
 
-        $controller = new ClientController($clients, $validator);
+        $controller = new ClientController(
+            $clients, $validator, $redirectRule
+        );
 
         $this->assertEquals('response', $controller->update($request, 1));
     }
@@ -112,7 +123,9 @@ public function test_404_response_if_client_doesnt_belong_to_user()
 
         $validator = m::mock('Illuminate\Contracts\Validation\Factory');
 
-        $controller = new ClientController($clients, $validator);
+        $controller = new ClientController(
+            $clients, $validator, m::mock(RedirectRule::class)
+        );
 
         $this->assertEquals(404, $controller->update($request, 1)->status());
     }
@@ -138,7 +151,9 @@ public function test_clients_can_be_deleted()
 
         $validator = m::mock('Illuminate\Contracts\Validation\Factory');
 
-        $controller = new ClientController($clients, $validator);
+        $controller = new ClientController(
+            $clients, $validator, m::mock(RedirectRule::class)
+        );
 
         $controller->destroy($request, 1);
     }
@@ -161,7 +176,9 @@ public function test_404_response_if_client_doesnt_belong_to_user_on_delete()
 
         $validator = m::mock('Illuminate\Contracts\Validation\Factory');
 
-        $controller = new ClientController($clients, $validator);
+        $controller = new ClientController(
+            $clients, $validator, m::mock(RedirectRule::class)
+        );
 
         $this->assertEquals(404, $controller->destroy($request, 1)->status());
     }

tests/RedirectRuleTest.php

@@ -0,0 +1,49 @@
+<?php
+
+namespace Laravel\Passport\Tests;
+
+use Illuminate\Contracts\Validation\Factory;
+use Illuminate\Contracts\Validation\Validator;
+use Mockery as m;
+use PHPUnit\Framework\TestCase;
+use Laravel\Passport\Http\Rules\RedirectRule;
+
+class RedirectRuleTest extends TestCase
+{
+    public function tearDown()
+    {
+        m::close();
+    }
+
+    public function test_it_passes_with_a_single_valid_url()
+    {
+        $rule = $this->rule($fails = false);
+
+        $this->assertTrue($rule->passes('redirect', 'https://example.com'));
+    }
+
+    public function test_it_passes_with_multiple_valid_urls()
+    {
+        $rule = $this->rule($fails = false);
+
+        $this->assertTrue($rule->passes('redirect', 'https://example.com,https://example2.com'));
+    }
+
+    public function test_it_fails_with_a_single_invalid_url()
+    {
+        $rule = $this->rule($fails = true);
+
+        $this->assertFalse($rule->passes('redirect', 'https://example.com,invalid'));
+    }
+
+    private function rule(bool $fails): RedirectRule
+    {
+        $validator = m::mock(Validator::class);
+        $validator->shouldReceive('fails')->andReturn($fails);
+
+        $factory = m::mock(Factory::class);
+        $factory->shouldReceive('make')->andReturn($validator);
+
+        return new RedirectRule($factory);
+    }
+}