Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.8] Support public clients #1052

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

use Illuminate\Support\Facades\Schema;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Database\Migrations\Migration;

class MakeClientSecretNullable extends Migration
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration isn't 100% necessary as we could just use the empty string for public clients but it seems more explicit to use NULL.

{
/**
* Run the migrations.
*
* @return void
*/
public function up()
{
Schema::table('oauth_clients', function (Blueprint $table) {
$table->string('secret', 100)->nullable()->change();
});
}

/**
* Reverse the migrations.
*
* @return void
*/
public function down()
{
Schema::table('oauth_clients', function (Blueprint $table) {
$table->string('secret', 100)->change();
});
}
}
19 changes: 18 additions & 1 deletion resources/js/components/Clients.vue
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,22 @@
</span>
</div>
</div>

<!-- Confidential -->
<div class="form-group row">
<label class="col-md-3 col-form-label">Confidential</label>

<div class="col-md-9">
<div class="checkbox">
<label>
<input type="checkbox" v-model="createForm.confidential">
</label>
</div>
<span class="form-text text-muted">
Require the client to authenticate with a secret.
</span>
</div>
</div>
</form>
</div>

Expand Down Expand Up @@ -222,7 +238,8 @@
createForm: {
errors: [],
name: '',
redirect: ''
redirect: '',
confidential: true
},

editForm: {
Expand Down
6 changes: 2 additions & 4 deletions src/Bridge/ClientRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public function getClientEntity($clientIdentifier)
}

return new Client(
$clientIdentifier, $record->name, $record->redirect, ! is_null($record->secret)
$clientIdentifier, $record->name, $record->redirect, $record->confidential()
);
}

Expand All @@ -52,7 +52,7 @@ public function validateClient($clientIdentifier, $clientSecret, $grantType)
return false;
}

return hash_equals($record->secret, (string) $clientSecret);
return $record->confidential() && hash_equals($record->secret, (string) $clientSecret);
}

/**
Expand All @@ -75,8 +75,6 @@ protected function handlesGrant($record, $grantType)
return $record->personal_access_client;
case 'password':
return $record->password_client;
case 'client_credentials':
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check was moved up to validateClient as the League server should only call validateClient for confidential clients, regardless of the grant type.

return ! empty($record->secret);
default:
return true;
}
Expand Down
10 changes: 10 additions & 0 deletions src/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,14 @@ public function firstParty()
{
return $this->personal_access_client || $this->password_client;
}

/**
* Determine if the client is a confidential client.
*
* @return bool
*/
public function confidential()
{
return ! empty($this->secret);
}
}
9 changes: 7 additions & 2 deletions src/ClientRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,19 @@ public function personalAccessClient()
* @param string $redirect
* @param bool $personalAccess
* @param bool $password
* @param bool $confidential
* @return \Laravel\Passport\Client
*/
public function create($userId, $name, $redirect, $personalAccess = false, $password = false)
public function create($userId, $name, $redirect, $personalAccess = false, $password = false, $confidential = true)
{
if ($personalAccess) {
$confidential = true;
}

$client = Passport::client()->forceFill([
'user_id' => $userId,
'name' => $name,
'secret' => Str::random(40),
'secret' => $confidential ? Str::random(40) : null,
'redirect' => $redirect,
'personal_access_client' => $personalAccess,
'password_client' => $password,
Expand Down
4 changes: 3 additions & 1 deletion src/Http/Controllers/ClientController.php
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,12 @@ public function store(Request $request)
$this->validation->make($request->all(), [
'name' => 'required|max:255',
'redirect' => ['required', $this->redirectRule],
'confidential' => 'boolean',
])->validate();

return $this->clients->create(
$request->user()->getKey(), $request->name, $request->redirect
$request->user()->getKey(), $request->name, $request->redirect,
false, false, (bool) $request->input('confidential', true)
)->makeVisible('secret');
}

Expand Down
5 changes: 5 additions & 0 deletions tests/BridgeClientRepositoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,9 @@ public function firstParty()
{
return $this->personal_access_client || $this->password_client;
}

public function confidential()
{
return ! empty($this->secret);
}
}
42 changes: 41 additions & 1 deletion tests/ClientControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public function test_clients_can_be_stored()

$clients->shouldReceive('create')
->once()
->with(1, 'client name', 'http://localhost')
->with(1, 'client name', 'http://localhost', false, false, true)
->andReturn($client = new Client);

$redirectRule = m::mock(RedirectRule::class);
Expand All @@ -60,6 +60,46 @@ public function test_clients_can_be_stored()
], [
'name' => 'required|max:255',
'redirect' => ['required', $redirectRule],
'confidential' => 'boolean',
])->andReturn($validator);
$validator->shouldReceive('validate')->once();

$controller = new ClientController(
$clients, $validator, $redirectRule
);

$this->assertEquals($client, $controller->store($request));
}

public function test_public_clients_can_be_stored()
{
$clients = m::mock(ClientRepository::class);

$request = Request::create(
'/',
'GET',
['name' => 'client name', 'redirect' => 'http://localhost', 'confidential' => false]
);
$request->setUserResolver(function () {
return new ClientControllerFakeUser;
});

$clients->shouldReceive('create')
->once()
->with(1, 'client name', 'http://localhost', false, false, false)
->andReturn($client = new Client);

$redirectRule = m::mock(RedirectRule::class);

$validator = m::mock(Factory::class);
$validator->shouldReceive('make')->once()->with([
'name' => 'client name',
'redirect' => 'http://localhost',
'confidential' => false,
], [
'name' => 'required|max:255',
'redirect' => ['required', $redirectRule],
'confidential' => 'boolean',
])->andReturn($validator);
$validator->shouldReceive('validate')->once();

Expand Down