From 0689f6f8f5c2811d69808a58c30654708bf54b52 Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Wed, 7 Aug 2024 16:28:46 +0330 Subject: [PATCH 1/6] fix skip auth if user already consented --- src/Client.php | 5 ++-- .../Controllers/AuthorizationController.php | 29 ++++++++++--------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/Client.php b/src/Client.php index ab2bc27d..f211670d 100644 --- a/src/Client.php +++ b/src/Client.php @@ -2,6 +2,7 @@ namespace Laravel\Passport; +use Illuminate\Contracts\Auth\Authenticatable; use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; @@ -163,9 +164,9 @@ public function firstParty() /** * Determine if the client should skip the authorization prompt. * - * @return bool + * @param \Laravel\Passport\Scope[] $scopes */ - public function skipsAuthorization() + public function skipsAuthorization(Authenticatable $user, array $scopes): bool { return false; } diff --git a/src/Http/Controllers/AuthorizationController.php b/src/Http/Controllers/AuthorizationController.php index 318cede4..137e651a 100644 --- a/src/Http/Controllers/AuthorizationController.php +++ b/src/Http/Controllers/AuthorizationController.php @@ -4,13 +4,13 @@ use Illuminate\Contracts\Auth\StatefulGuard; use Illuminate\Http\Request; +use Illuminate\Support\Facades\Date; use Illuminate\Support\Str; use Laravel\Passport\Bridge\User; use Laravel\Passport\ClientRepository; use Laravel\Passport\Contracts\AuthorizationViewResponse; use Laravel\Passport\Exceptions\AuthenticationException; use Laravel\Passport\Passport; -use Laravel\Passport\TokenRepository; use League\OAuth2\Server\AuthorizationServer; use League\OAuth2\Server\Exception\OAuthServerException; use Nyholm\Psr7\Response as Psr7Response; @@ -67,10 +67,7 @@ public function __construct(AuthorizationServer $server, * @param \Laravel\Passport\TokenRepository $tokens * @return \Illuminate\Http\Response|\Laravel\Passport\Contracts\AuthorizationViewResponse */ - public function authorize(ServerRequestInterface $psrRequest, - Request $request, - ClientRepository $clients, - TokenRepository $tokens) + public function authorize(ServerRequestInterface $psrRequest, Request $request, ClientRepository $clients) { $authRequest = $this->withErrorHandling(function () use ($psrRequest) { return $this->server->validateAuthorizationRequest($psrRequest); @@ -78,8 +75,8 @@ public function authorize(ServerRequestInterface $psrRequest, if ($this->guard->guest()) { return $request->get('prompt') === 'none' - ? $this->denyRequest($authRequest) - : $this->promptForLogin($request); + ? $this->denyRequest($authRequest) + : $this->promptForLogin($request); } if ($request->get('prompt') === 'login' && @@ -98,7 +95,7 @@ public function authorize(ServerRequestInterface $psrRequest, $client = $clients->find($authRequest->getClient()->getIdentifier()); if ($request->get('prompt') !== 'consent' && - ($client->skipsAuthorization() || $this->hasValidToken($tokens, $user, $client, $scopes))) { + ($client->skipsAuthorization($user, $scopes) || $this->hasConsent($user, $client, $scopes))) { return $this->approveRequest($authRequest, $user); } @@ -134,19 +131,23 @@ protected function parseScopes($authRequest) } /** - * Determine if a valid token exists for the given user, client, and scopes. + * Determine if the given user has already consented the scopes for the client. * - * @param \Laravel\Passport\TokenRepository $tokens * @param \Illuminate\Contracts\Auth\Authenticatable $user * @param \Laravel\Passport\Client $client * @param array $scopes * @return bool */ - protected function hasValidToken($tokens, $user, $client, $scopes) + protected function hasConsent($user, $client, $scopes) { - $token = $tokens->findValidToken($user, $client); - - return $token && $token->scopes === collect($scopes)->pluck('id')->all(); + return collect($scopes)->pluck('id') + ->diff($user->tokens() + ->where('client_id', $client->getKey()) + ->where('revoked', false) + ->where('expires_at', '>', Date::now()) + ->pluck('scopes') + ->flatten() + )->isEmpty(); } /** From ac9c180d38645645c2684b4087b731c2c776a143 Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Wed, 7 Aug 2024 16:28:55 +0330 Subject: [PATCH 2/6] fix tests --- tests/Unit/AuthorizationControllerTest.php | 58 ++++++++-------------- 1 file changed, 21 insertions(+), 37 deletions(-) diff --git a/tests/Unit/AuthorizationControllerTest.php b/tests/Unit/AuthorizationControllerTest.php index 12e70335..fb648c73 100644 --- a/tests/Unit/AuthorizationControllerTest.php +++ b/tests/Unit/AuthorizationControllerTest.php @@ -2,6 +2,7 @@ namespace Laravel\Passport\Tests\Unit; +use Illuminate\Contracts\Auth\Authenticatable; use Illuminate\Contracts\Auth\StatefulGuard; use Illuminate\Http\Request; use Laravel\Passport\Bridge\Scope; @@ -12,8 +13,6 @@ use Laravel\Passport\Http\Controllers\AuthorizationController; use Laravel\Passport\Http\Responses\AuthorizationViewResponse; use Laravel\Passport\Passport; -use Laravel\Passport\Token; -use Laravel\Passport\TokenRepository; use League\OAuth2\Server\AuthorizationServer; use League\OAuth2\Server\Exception\OAuthServerException as LeagueException; use League\OAuth2\Server\RequestTypes\AuthorizationRequest; @@ -44,7 +43,7 @@ public function test_authorization_view_is_presented() $controller = new AuthorizationController($server, $guard, $response); $guard->shouldReceive('guest')->andReturn(false); - $guard->shouldReceive('user')->andReturn($user = m::mock()); + $guard->shouldReceive('user')->andReturn($user = m::mock(Authenticatable::class)); $server->shouldReceive('validateAuthorizationRequest')->andReturn($authRequest = m::mock(AuthorizationRequestInterface::class)); $request = m::mock(Request::class); @@ -60,9 +59,9 @@ public function test_authorization_view_is_presented() $clients = m::mock(ClientRepository::class); $clients->shouldReceive('find')->with(1)->andReturn($client = m::mock(Client::class)); $client->shouldReceive('skipsAuthorization')->andReturn(false); + $client->shouldReceive('getKey')->andReturn(1); - $tokens = m::mock(TokenRepository::class); - $tokens->shouldReceive('findValidToken')->with($user, $client)->andReturnNull(); + $user->shouldReceive('tokens->where->where->where->pluck->flatten')->andReturn(collect()); $response->shouldReceive('withParameters')->once()->andReturnUsing(function ($data) use ($client, $user, $request) { $this->assertEquals($client, $data['client']); @@ -74,7 +73,7 @@ public function test_authorization_view_is_presented() }); $this->assertSame('view', $controller->authorize( - m::mock(ServerRequestInterface::class), $request, $clients, $tokens + m::mock(ServerRequestInterface::class), $request, $clients )); } @@ -93,12 +92,11 @@ public function test_authorization_exceptions_are_handled() $request->shouldReceive('session')->andReturn($session = m::mock()); $clients = m::mock(ClientRepository::class); - $tokens = m::mock(TokenRepository::class); $this->expectException(OAuthServerException::class); $controller->authorize( - m::mock(ServerRequestInterface::class), $request, $clients, $tokens + m::mock(ServerRequestInterface::class), $request, $clients ); } @@ -115,7 +113,7 @@ public function test_request_is_approved_if_valid_token_exists() $controller = new AuthorizationController($server, $guard, $response); $guard->shouldReceive('guest')->andReturn(false); - $guard->shouldReceive('user')->andReturn($user = m::mock()); + $guard->shouldReceive('user')->andReturn($user = m::mock(Authenticatable::class)); $psrResponse = new Response(); $psrResponse->getBody()->write('approved'); $server->shouldReceive('validateAuthorizationRequest') @@ -140,15 +138,12 @@ public function test_request_is_approved_if_valid_token_exists() $clients->shouldReceive('find')->with(1)->andReturn($client = m::mock(Client::class)); $client->shouldReceive('skipsAuthorization')->andReturn(false); + $client->shouldReceive('getKey')->andReturn(1); - $tokens = m::mock(TokenRepository::class); - $tokens->shouldReceive('findValidToken') - ->with($user, $client) - ->andReturn($token = m::mock(Token::class)); - $token->shouldReceive('getAttribute')->with('scopes')->andReturn(['scope-1']); + $user->shouldReceive('tokens->where->where->where->pluck->flatten')->andReturn(collect(['scope-1'])); $this->assertSame('approved', $controller->authorize( - m::mock(ServerRequestInterface::class), $request, $clients, $tokens + m::mock(ServerRequestInterface::class), $request, $clients )->getContent()); } @@ -165,7 +160,7 @@ public function test_request_is_approved_if_client_can_skip_authorization() $controller = new AuthorizationController($server, $guard, $response); $guard->shouldReceive('guest')->andReturn(false); - $guard->shouldReceive('user')->andReturn($user = m::mock()); + $guard->shouldReceive('user')->andReturn($user = m::mock(Authenticatable::class)); $psrResponse = new Response(); $psrResponse->getBody()->write('approved'); $server->shouldReceive('validateAuthorizationRequest') @@ -191,13 +186,10 @@ public function test_request_is_approved_if_client_can_skip_authorization() $client->shouldReceive('skipsAuthorization')->andReturn(true); - $tokens = m::mock(TokenRepository::class); - $tokens->shouldReceive('findValidToken') - ->with($user, $client) - ->andReturnNull(); + $user->shouldReceive('tokens->where->where->where->pluck->flatten')->andReturn(collect()); $this->assertSame('approved', $controller->authorize( - m::mock(ServerRequestInterface::class), $request, $clients, $tokens + m::mock(ServerRequestInterface::class), $request, $clients )->getContent()); } @@ -232,9 +224,6 @@ public function test_authorization_view_is_presented_if_request_has_prompt_equal $clients->shouldReceive('find')->with(1)->andReturn($client = m::mock(Client::class)); $client->shouldReceive('skipsAuthorization')->andReturn(false); - $tokens = m::mock(TokenRepository::class); - $tokens->shouldNotReceive('findValidToken'); - $response->shouldReceive('withParameters')->once()->andReturnUsing(function ($data) use ($client, $user, $request) { $this->assertEquals($client, $data['client']); $this->assertEquals($user, $data['user']); @@ -245,7 +234,7 @@ public function test_authorization_view_is_presented_if_request_has_prompt_equal }); $this->assertSame('view', $controller->authorize( - m::mock(ServerRequestInterface::class), $request, $clients, $tokens + m::mock(ServerRequestInterface::class), $request, $clients )); } @@ -264,7 +253,7 @@ public function test_authorization_denied_if_request_has_prompt_equals_to_none() $controller = new AuthorizationController($server, $guard, $response); $guard->shouldReceive('guest')->andReturn(false); - $guard->shouldReceive('user')->andReturn($user = m::mock()); + $guard->shouldReceive('user')->andReturn($user = m::mock(Authenticatable::class)); $server->shouldReceive('validateAuthorizationRequest') ->andReturn($authRequest = m::mock(AuthorizationRequest::class)); $server->shouldReceive('completeAuthorizationRequest') @@ -288,14 +277,12 @@ public function test_authorization_denied_if_request_has_prompt_equals_to_none() $clients = m::mock(ClientRepository::class); $clients->shouldReceive('find')->with(1)->andReturn($client = m::mock(Client::class)); $client->shouldReceive('skipsAuthorization')->andReturn(false); + $client->shouldReceive('getKey')->andReturn(1); - $tokens = m::mock(TokenRepository::class); - $tokens->shouldReceive('findValidToken') - ->with($user, $client) - ->andReturnNull(); + $user->shouldReceive('tokens->where->where->where->pluck->flatten')->andReturn(collect()); $controller->authorize( - m::mock(ServerRequestInterface::class), $request, $clients, $tokens + m::mock(ServerRequestInterface::class), $request, $clients ); } @@ -324,11 +311,10 @@ public function test_authorization_denied_if_unauthenticated_and_request_has_pro $authRequest->shouldReceive('getGrantTypeId')->andReturn('authorization_code'); $clients = m::mock(ClientRepository::class); - $tokens = m::mock(TokenRepository::class); try { $controller->authorize( - m::mock(ServerRequestInterface::class), $request, $clients, $tokens + m::mock(ServerRequestInterface::class), $request, $clients ); } catch (\Laravel\Passport\Exceptions\OAuthServerException $e) { $this->assertStringStartsWith( @@ -362,10 +348,9 @@ public function test_logout_and_prompt_login_if_request_has_prompt_equals_to_log $request->shouldReceive('get')->with('prompt')->andReturn('login'); $clients = m::mock(ClientRepository::class); - $tokens = m::mock(TokenRepository::class); $controller->authorize( - m::mock(ServerRequestInterface::class), $request, $clients, $tokens + m::mock(ServerRequestInterface::class), $request, $clients ); } @@ -390,10 +375,9 @@ public function test_user_should_be_authenticated() $request->shouldReceive('get')->with('prompt')->andReturn(null); $clients = m::mock(ClientRepository::class); - $tokens = m::mock(TokenRepository::class); $controller->authorize( - m::mock(ServerRequestInterface::class), $request, $clients, $tokens + m::mock(ServerRequestInterface::class), $request, $clients ); } } From 25060782c48de3230633c8d006d89e54682a919f Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Wed, 7 Aug 2024 16:32:09 +0330 Subject: [PATCH 3/6] formatting --- src/Http/Controllers/AuthorizationController.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Http/Controllers/AuthorizationController.php b/src/Http/Controllers/AuthorizationController.php index 137e651a..7538eeba 100644 --- a/src/Http/Controllers/AuthorizationController.php +++ b/src/Http/Controllers/AuthorizationController.php @@ -64,7 +64,6 @@ public function __construct(AuthorizationServer $server, * @param \Psr\Http\Message\ServerRequestInterface $psrRequest * @param \Illuminate\Http\Request $request * @param \Laravel\Passport\ClientRepository $clients - * @param \Laravel\Passport\TokenRepository $tokens * @return \Illuminate\Http\Response|\Laravel\Passport\Contracts\AuthorizationViewResponse */ public function authorize(ServerRequestInterface $psrRequest, Request $request, ClientRepository $clients) From b8536ad0ba52e4eef731ebc373e489f10cab6b8e Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Wed, 7 Aug 2024 17:36:20 +0330 Subject: [PATCH 4/6] formatting --- src/Http/Controllers/AuthorizationController.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Http/Controllers/AuthorizationController.php b/src/Http/Controllers/AuthorizationController.php index 7538eeba..7c12ce94 100644 --- a/src/Http/Controllers/AuthorizationController.php +++ b/src/Http/Controllers/AuthorizationController.php @@ -94,7 +94,7 @@ public function authorize(ServerRequestInterface $psrRequest, Request $request, $client = $clients->find($authRequest->getClient()->getIdentifier()); if ($request->get('prompt') !== 'consent' && - ($client->skipsAuthorization($user, $scopes) || $this->hasConsent($user, $client, $scopes))) { + ($client->skipsAuthorization($user, $scopes) || $this->hasGrantedScopes($user, $client, $scopes))) { return $this->approveRequest($authRequest, $user); } @@ -130,14 +130,14 @@ protected function parseScopes($authRequest) } /** - * Determine if the given user has already consented the scopes for the client. + * Determine if the given user has already granted the client access to the scopes. * * @param \Illuminate\Contracts\Auth\Authenticatable $user * @param \Laravel\Passport\Client $client * @param array $scopes * @return bool */ - protected function hasConsent($user, $client, $scopes) + protected function hasGrantedScopes($user, $client, $scopes) { return collect($scopes)->pluck('id') ->diff($user->tokens() From 5d88d423c50762a6c2255ab0cbeec52772afae14 Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Wed, 7 Aug 2024 17:43:34 +0330 Subject: [PATCH 5/6] formatting --- src/Http/Controllers/AuthorizationController.php | 15 +++++++-------- tests/Unit/AuthorizationControllerTest.php | 8 +++----- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/Http/Controllers/AuthorizationController.php b/src/Http/Controllers/AuthorizationController.php index 7c12ce94..37bbb9fb 100644 --- a/src/Http/Controllers/AuthorizationController.php +++ b/src/Http/Controllers/AuthorizationController.php @@ -139,14 +139,13 @@ protected function parseScopes($authRequest) */ protected function hasGrantedScopes($user, $client, $scopes) { - return collect($scopes)->pluck('id') - ->diff($user->tokens() - ->where('client_id', $client->getKey()) - ->where('revoked', false) - ->where('expires_at', '>', Date::now()) - ->pluck('scopes') - ->flatten() - )->isEmpty(); + return collect($scopes)->pluck('id')->diff( + $user->tokens()->where([ + ['client_id', '=', $client->getKey()], + ['revoked', '=', false], + ['expires_at', '>', Date::now()], + ])->pluck('scopes')->flatten() + )->isEmpty(); } /** diff --git a/tests/Unit/AuthorizationControllerTest.php b/tests/Unit/AuthorizationControllerTest.php index fb648c73..3ebeed5a 100644 --- a/tests/Unit/AuthorizationControllerTest.php +++ b/tests/Unit/AuthorizationControllerTest.php @@ -61,7 +61,7 @@ public function test_authorization_view_is_presented() $client->shouldReceive('skipsAuthorization')->andReturn(false); $client->shouldReceive('getKey')->andReturn(1); - $user->shouldReceive('tokens->where->where->where->pluck->flatten')->andReturn(collect()); + $user->shouldReceive('tokens->where->pluck->flatten')->andReturn(collect()); $response->shouldReceive('withParameters')->once()->andReturnUsing(function ($data) use ($client, $user, $request) { $this->assertEquals($client, $data['client']); @@ -140,7 +140,7 @@ public function test_request_is_approved_if_valid_token_exists() $client->shouldReceive('skipsAuthorization')->andReturn(false); $client->shouldReceive('getKey')->andReturn(1); - $user->shouldReceive('tokens->where->where->where->pluck->flatten')->andReturn(collect(['scope-1'])); + $user->shouldReceive('tokens->where->pluck->flatten')->andReturn(collect(['scope-1'])); $this->assertSame('approved', $controller->authorize( m::mock(ServerRequestInterface::class), $request, $clients @@ -186,8 +186,6 @@ public function test_request_is_approved_if_client_can_skip_authorization() $client->shouldReceive('skipsAuthorization')->andReturn(true); - $user->shouldReceive('tokens->where->where->where->pluck->flatten')->andReturn(collect()); - $this->assertSame('approved', $controller->authorize( m::mock(ServerRequestInterface::class), $request, $clients )->getContent()); @@ -279,7 +277,7 @@ public function test_authorization_denied_if_request_has_prompt_equals_to_none() $client->shouldReceive('skipsAuthorization')->andReturn(false); $client->shouldReceive('getKey')->andReturn(1); - $user->shouldReceive('tokens->where->where->where->pluck->flatten')->andReturn(collect()); + $user->shouldReceive('tokens->where->pluck->flatten')->andReturn(collect()); $controller->authorize( m::mock(ServerRequestInterface::class), $request, $clients From e84df18d2f6413639e0af5f3e8fac56c7af813b5 Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Wed, 7 Aug 2024 19:27:39 +0330 Subject: [PATCH 6/6] formatting --- src/Http/Controllers/AuthorizationController.php | 4 ++-- tests/Unit/AuthorizationControllerTest.php | 10 ++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Http/Controllers/AuthorizationController.php b/src/Http/Controllers/AuthorizationController.php index 37bbb9fb..87491199 100644 --- a/src/Http/Controllers/AuthorizationController.php +++ b/src/Http/Controllers/AuthorizationController.php @@ -140,8 +140,8 @@ protected function parseScopes($authRequest) protected function hasGrantedScopes($user, $client, $scopes) { return collect($scopes)->pluck('id')->diff( - $user->tokens()->where([ - ['client_id', '=', $client->getKey()], + $client->tokens()->where([ + ['user_id', '=', $user->getAuthIdentifier()], ['revoked', '=', false], ['expires_at', '>', Date::now()], ])->pluck('scopes')->flatten() diff --git a/tests/Unit/AuthorizationControllerTest.php b/tests/Unit/AuthorizationControllerTest.php index 3ebeed5a..b57d0a1d 100644 --- a/tests/Unit/AuthorizationControllerTest.php +++ b/tests/Unit/AuthorizationControllerTest.php @@ -59,9 +59,9 @@ public function test_authorization_view_is_presented() $clients = m::mock(ClientRepository::class); $clients->shouldReceive('find')->with(1)->andReturn($client = m::mock(Client::class)); $client->shouldReceive('skipsAuthorization')->andReturn(false); - $client->shouldReceive('getKey')->andReturn(1); + $client->shouldReceive('tokens->where->pluck->flatten')->andReturn(collect()); - $user->shouldReceive('tokens->where->pluck->flatten')->andReturn(collect()); + $user->shouldReceive('getAuthIdentifier')->andReturn(1); $response->shouldReceive('withParameters')->once()->andReturnUsing(function ($data) use ($client, $user, $request) { $this->assertEquals($client, $data['client']); @@ -139,8 +139,7 @@ public function test_request_is_approved_if_valid_token_exists() $client->shouldReceive('skipsAuthorization')->andReturn(false); $client->shouldReceive('getKey')->andReturn(1); - - $user->shouldReceive('tokens->where->pluck->flatten')->andReturn(collect(['scope-1'])); + $client->shouldReceive('tokens->where->pluck->flatten')->andReturn(collect(['scope-1'])); $this->assertSame('approved', $controller->authorize( m::mock(ServerRequestInterface::class), $request, $clients @@ -276,8 +275,7 @@ public function test_authorization_denied_if_request_has_prompt_equals_to_none() $clients->shouldReceive('find')->with(1)->andReturn($client = m::mock(Client::class)); $client->shouldReceive('skipsAuthorization')->andReturn(false); $client->shouldReceive('getKey')->andReturn(1); - - $user->shouldReceive('tokens->where->pluck->flatten')->andReturn(collect()); + $client->shouldReceive('tokens->where->pluck->flatten')->andReturn(collect()); $controller->authorize( m::mock(ServerRequestInterface::class), $request, $clients