From e09c4bf9dbdab67b6886fac54da7c6620ffa871d Mon Sep 17 00:00:00 2001 From: Enrico Zimuel Date: Thu, 16 Jul 2015 19:01:27 +0200 Subject: [PATCH] Do not use a user_id provided in a query string The practice leads to the ability for an attacker to authenticate as one user, but then spoof as another user on later, authenticated requests. --- config/module.config.php | 7 +--- src/Provider/UserId/AuthenticationService.php | 39 +++++++++++++++++-- .../UserId/AuthenticationServiceFactory.php | 11 +++++- src/Provider/UserId/Request.php | 24 ------------ test/Controller/AuthControllerTest.php | 3 +- ...ollerWithZendAuthenticationServiceTest.php | 2 +- 6 files changed, 50 insertions(+), 36 deletions(-) delete mode 100644 src/Provider/UserId/Request.php diff --git a/config/module.config.php b/config/module.config.php index 7ab8994..8861ba3 100644 --- a/config/module.config.php +++ b/config/module.config.php @@ -51,10 +51,7 @@ ), 'service_manager' => array( 'aliases' => array( - 'ZF\OAuth2\Provider\UserId' => 'ZF\OAuth2\Provider\UserId\Request', - ), - 'invokables' => array( - 'ZF\OAuth2\Provider\UserId\Request' => 'ZF\OAuth2\Provider\UserId\Request', + 'ZF\OAuth2\Provider\UserId' => 'ZF\OAuth2\Provider\UserId\AuthenticationService', ), 'factories' => array( 'ZF\OAuth2\Adapter\PdoAdapter' => 'ZF\OAuth2\Factory\PdoAdapterFactory', @@ -98,7 +95,7 @@ * * If true, client errors are returned using the * application/problem+json content type, - * otherwise in the format described in the oauth2 specification + * otherwise in the format described in the oauth2 specification * (default: true) */ 'api_problem_error_response' => true, diff --git a/src/Provider/UserId/AuthenticationService.php b/src/Provider/UserId/AuthenticationService.php index f14c13d..c49ff33 100644 --- a/src/Provider/UserId/AuthenticationService.php +++ b/src/Provider/UserId/AuthenticationService.php @@ -16,24 +16,57 @@ class AuthenticationService implements UserIdProviderInterface */ private $authenticationService; + /** + * @var string + */ + private $userId = 'id'; + /** * Set authentication service * * @param ZendAuthenticationService $service + * @param array $config */ - public function __construct(ZendAuthenticationService $service) + public function __construct(ZendAuthenticationService $service = null, $config = array()) { $this->authenticationService = $service; + + if (isset($config['zf-oauth2']['user_id'])) { + $this->userId = $config['zf-oauth2']['user_id']; + } } /** * Use Zend\Authentication\AuthenticationService to fetch the identity. * - * @param RequestInterface $request + * @param RequestInterface $request * @return mixed */ public function __invoke(RequestInterface $request) { - return $this->authenticationService->getIdentity(); + if (empty($this->authenticationService)) { + return null; + } + + $identity = $this->authenticationService->getIdentity(); + + if (is_object($identity)) { + if (property_exists($identity, $this->userId)) { + return $identity->{$this->$userId}; + } + + $method = "get" . ucfirst($this->userId); + if (method_exists($identity, $method)) { + return $identity->$method(); + } + + return null; + } + + if (is_array($identity) && isset($identity[$this->userId])) { + return $identity[$this->userId]; + } + + return null; } } diff --git a/src/Provider/UserId/AuthenticationServiceFactory.php b/src/Provider/UserId/AuthenticationServiceFactory.php index cdaa5b4..1fed487 100644 --- a/src/Provider/UserId/AuthenticationServiceFactory.php +++ b/src/Provider/UserId/AuthenticationServiceFactory.php @@ -17,6 +17,15 @@ class AuthenticationServiceFactory implements FactoryInterface */ public function createService(ServiceLocatorInterface $services) { - return new AuthenticationService($services->get('Zend\Authentication\AuthenticationService')); + $config = $services->get('Config'); + + if ($services->has('Zend\Authentication\AuthenticationService')) { + return new AuthenticationService( + $services->get('Zend\Authentication\AuthenticationService'), + $config + ); + } + + return new AuthenticationService(null, $config); } } diff --git a/src/Provider/UserId/Request.php b/src/Provider/UserId/Request.php deleted file mode 100644 index bb234d0..0000000 --- a/src/Provider/UserId/Request.php +++ /dev/null @@ -1,24 +0,0 @@ -getQuery('user_id', null); - } -} diff --git a/test/Controller/AuthControllerTest.php b/test/Controller/AuthControllerTest.php index 99d5c25..a335913 100644 --- a/test/Controller/AuthControllerTest.php +++ b/test/Controller/AuthControllerTest.php @@ -172,7 +172,6 @@ public function testAuthorizeCode() 'response_type' => 'code', 'client_id' => 'testclient', 'state' => 'xyz', - 'user_id' => 123, 'redirect_uri' => '/oauth/receivecode', ))); $request->setPost(new Parameters(array( @@ -199,7 +198,7 @@ public function testAuthorizeCode() $selectString = $sql->getSqlStringForSqlObject($select); $results = $adapter->query($selectString, $adapter::QUERY_MODE_EXECUTE)->toArray(); - $this->assertEquals('123', $results[0]['user_id']); + $this->assertEquals(null, $results[0]['user_id']); // test get token from authorized code $request = $this->getRequest(); diff --git a/test/Controller/AuthControllerWithZendAuthenticationServiceTest.php b/test/Controller/AuthControllerWithZendAuthenticationServiceTest.php index f7c0d02..d54dca7 100644 --- a/test/Controller/AuthControllerWithZendAuthenticationServiceTest.php +++ b/test/Controller/AuthControllerWithZendAuthenticationServiceTest.php @@ -97,7 +97,7 @@ public function testAuthorizeCode() ->query($query) ->fetch(); - $this->assertEquals('123', $row['user_id']); + $this->assertEquals(null, $row['user_id']); // test get token from authorized code $request = $this->getRequest();