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

Release 5.1.0 #230

Merged
merged 7 commits into from
Mar 5, 2018
Merged
Show file tree
Hide file tree
Changes from 6 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
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,17 @@
# Change Log

## [5.1.0](https://github.com/auth0/auth0-PHP/tree/5.1.0) (2018-03-02)
[Full Changelog](https://github.com/auth0/auth0-PHP/compare/5.0.6...5.1.0)

**Closed issues**
- Support for php-jwt 5 [\#210](https://github.com/auth0/auth0-PHP/issues/210)

**Added**
- Adding tests for state handler; correcting storage method used [\#228](https://github.com/auth0/auth0-PHP/pull/228) ([joshcanhelp](https://github.com/joshcanhelp))

**Changed**
- Bumping JWT package version [\#229](https://github.com/auth0/auth0-PHP/pull/229) ([joshcanhelp](https://github.com/joshcanhelp))

## [5.0.6](https://github.com/auth0/auth0-PHP/tree/5.0.4) (2017-11-24)
[Full Changelog](https://github.com/auth0/auth0-PHP/compare/5.0.4...5.0.6)

Expand Down
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ Check our docs page to get a complete guide on how to install it in an existing

> If you find something wrong in our docs, PR are welcome in our docs repo: https://github.com/auth0/docs

## Security Upgrade Notes 5.1.0+

**State validation** is now default behaviour for improved security. By default this will automatically use **Session Storage** and will
Copy link
Contributor

Choose a reason for hiding this comment

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

probably not intended carriage return

apply if you are using the combination of the `Auth0->login()` method to call the `/authorize` endpoint and using any method which calls the `Auth0->exchange()` method in your callback.
If you require custom storage methods you can implement your own [StateHandler](https://github.com/auth0/auth0-PHP/blob/master/src/API/Helpers/State/StateHandler.php) and set it using the `state_handler` key when you initialize an `Auth0` instance.

**Important:** If you are using the `Auth0->exchange()` and using a method other than `Auth0->login()` to generate the Authorize URL you can disable the *StateHandler* by setting the `state_handler` key to `false` when you initialize the `Auth0` instance. However, it is **Highly Recommended** to implement state validation.

## Getting started

### Decoding and verifying tokens
Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"php": "^5.5 || ^7.0",
"guzzlehttp/guzzle": "~6.0",
"ext-json": "*",
"firebase/php-jwt" : "^4.0"
"firebase/php-jwt" : "^5.0"
},
"require-dev": {
"phpunit/phpunit": "^4.8 || ^5.7",
Expand Down
7 changes: 7 additions & 0 deletions examples/basic-webapp/index.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@
'persist_refresh_token' => true,
]);

if (isset($_REQUEST['logout'])) {
$auth0->logout();
session_destroy();
header("Location: /");
die();
}

$userInfo = $auth0->getUser();

if (!$userInfo) {
Expand Down
48 changes: 48 additions & 0 deletions src/API/Helpers/State/DummyStateHandler.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?php

namespace Auth0\SDK\API\Helpers\State;

/*
* This file is part of Auth0-PHP package.
*
* (c) Auth0
*
* For the full copyright and license information, please view the LICENSE file
* that was distributed with this source code.
*/

/**
* Dummy implementation of the StateHandler
*
* @author Auth0
*/
class DummyStateHandler implements StateHandler
{
Copy link
Contributor

Choose a reason for hiding this comment

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

class braces style is different from the method one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't be but not consistent in the library (yet)

/**
* Generate state value to be used for the state param value during authorization.
*
* @return string
*/
public function issue() {
return null;
}

/**
* Store state value to be used for the state param value during authorization.
*
*/
public function store($state) {
}

/**
* Perform validation of the returned state with the previously generated state.
*
* @param string $state
*
* @return bool result
* @throws exception
*/
public function validate($state) {
return true;
}
}
67 changes: 67 additions & 0 deletions src/API/Helpers/State/SessionStateHandler.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<?php

namespace Auth0\SDK\API\Helpers\State;

use Auth0\SDK\Store\SessionStore;
use Auth0\SDK\Exception\CoreException;

/*
* This file is part of Auth0-PHP package.
*
* (c) Auth0
*
* For the full copyright and license information, please view the LICENSE file
* that was distributed with this source code.
*/

/**
* Session based implementation of StateHandler.
*
* @author Auth0
*/
class SessionStateHandler implements StateHandler
{
const STATE_NAME = 'webauth_state';

private $store;

/**
* @param SessionStore $store
*/
public function __construct(SessionStore $store) {
$this->store = $store;
}

/**
* Generate state value to be used for the state param value during authorization.
*
* @return string
*/
public function issue() {
$state = uniqid('', true);
$this->store($state);
return $state;
}

/**
* Store a given state value to be used for the state param value during authorization.
*
* @return string
*/
public function store($state) {
$this->store->set(self::STATE_NAME, $state);
}

/**
* Perform validation of the returned state with the previously generated state.
*
* @param string $state
*
* @throws exception
*/
public function validate($state) {
$valid = $this->store->get(self::STATE_NAME) == $state;
$this->store->delete(self::STATE_NAME);
return $valid;
}
}
46 changes: 46 additions & 0 deletions src/API/Helpers/State/StateHandler.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<?php

namespace Auth0\SDK\API\Helpers\State;

use Auth0\SDK\Store\StoreInterface;

/*
* This file is part of Auth0-PHP package.
*
* (c) Auth0
*
* For the full copyright and license information, please view the LICENSE file
* that was distributed with this source code.
*/

/**
* This interface must be implemented by state handlers.
*
* @author Auth0
*/
interface StateHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

inconsistent brace style 😛


/**
* Generate state value to be used for the state param value during authorization.
*
* @return string || null
*/
public function issue();

/**
* Store a given state value to be used for the state param value during authorization.
*
* @return string
*/
public function store($state);

/**
* Perform validation of the returned state with the previously generated state.
*
* @param string $state
*
* @return bool result
* @throws exception
*/
public function validate($state);
}
2 changes: 2 additions & 0 deletions src/API/Management/Tenants.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ public function update($data)
return $this->apiClient->patch()
->tenants()
->settings()
->withHeader(new ContentType('application/json'))
->withBody(json_encode($data))
->call();
}
}
47 changes: 46 additions & 1 deletion src/Auth0.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
use Auth0\SDK\Store\SessionStore;
use Auth0\SDK\Store\StoreInterface;
use Auth0\SDK\API\Authentication;
use Auth0\SDK\API\Helpers\State\StateHandler;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see StateHandler being instantiated or referenced here. I do see SessionStateHandler and DummyStateHandler though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's mentioned in a docblock, which is flagged in my IDE

use Auth0\SDK\API\Helpers\State\SessionStateHandler;
use Auth0\SDK\API\Helpers\State\DummyStateHandler;

/**
* This class provides access to Auth0 Platform.
Expand Down Expand Up @@ -132,6 +135,12 @@ class Auth0 {

protected $guzzleOptions;

/**
* State Handler
* @var StateHandler
*/
protected $stateHandler;

/**
* BaseAuth0 Constructor.
*
Expand Down Expand Up @@ -229,6 +238,15 @@ public function __construct(array $config) {
} else {
$this->setStore(new SessionStore());
}
if (isset($config['state_handler'])) {
if ($config['state_handler'] === false) {
$this->stateHandler = new DummyStateHandler();
} else {
$this->stateHandler = $config['state_handler'];
}
} else {
$this->stateHandler = new SessionStateHandler(new SessionStore());
}

$this->authentication = new Authentication ($this->domain, $this->client_id, $this->client_secret, $this->audience, $this->scope, $this->guzzleOptions);

Expand All @@ -238,7 +256,8 @@ public function __construct(array $config) {
$this->refresh_token = $this->store->get("refresh_token");
}

public function login($state = null, $connection = null) {
public function login($state = null, $connection = null, $additional_params = []) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is NOT a breaking change because you're passing [] as default value when the parameter is missing. Am I correct?


$params = [];
if ($this->audience) {
$params['audience'] = $this->audience;
Expand All @@ -247,8 +266,18 @@ public function login($state = null, $connection = null) {
$params['scope'] = $this->scope;
}

if($state === null) {
$state = $this->stateHandler->issue();
} else {
$this->stateHandler->store($state);
}

$params['response_mode'] = $this->response_mode;

if($additional_params) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if my statement is correct, then this will always be either [] or some (I say good) value passed by the user. In any case, this if is always true. Maybe the check should be if($additional_params && $additional_params.length/size>0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That if will check for truth-y values so an empty array or null will skip it. That said, if someone passed in a 1 or a string or something, they'll get an error. Probably should be something like:

if(!empty($additional_params) && is_array($additional_params)) {
      $params = array_replace($params, $additional_params);
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

I will accept the change, however as long as the parameter type is documented somewhere, users shouldn't be passing invalid (non-array) values. So next time: doc + if($var) should be enough.

$params = array_replace($params, $additional_params);
}

$url = $this->authentication->get_authorize_link($this->response_type, $this->redirect_uri, $connection, $state, $params);

header("Location: $url");
Expand Down Expand Up @@ -298,6 +327,12 @@ public function exchange() {
return false;
}

$state = $this->getState();

if (!$this->stateHandler->validate($state)) {
throw new CoreException('Invalid state');
}

if ($this->user) {
throw new CoreException('Can\'t initialize a new session while there is one active session already');
}
Expand Down Expand Up @@ -387,6 +422,16 @@ protected function getAuthorizationCode() {
return null;
}

protected function getState() {
if ($this->response_mode === 'query') {
return (isset($_GET['state']) ? $_GET['state'] : null);
} elseif ($this->response_mode === 'form_post') {
return (isset($_POST['state']) ? $_POST['state'] : null);
}

return null;
}

public function logout() {
$this->deleteAllPersistentData();
$this->access_token = NULL;
Expand Down
2 changes: 0 additions & 2 deletions src/Store/SessionStore.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
* For the full copyright and license information, please view the LICENSE file
* that was distributed with this source code.
*/
use Auth0\SDK\API\Oauth2Client;


/**
* This class provides a layer to persist user access using PHP Sessions.
Expand Down
40 changes: 40 additions & 0 deletions tests/API/Helpers/State/StateHandlerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php
namespace Auth0\Tests\Api\Helpers\State;

use Auth0\SDK\API\Helpers\State\SessionStateHandler;
use Auth0\SDK\Store\SessionStore;

class StateHandlerTest extends \PHPUnit_Framework_TestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

is the \ intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHP namespace thing, means 'use the global namespace'


public function testStateStoredCorrectly() {
// Suppress header sent error
@$test_store = new SessionStore();
$test_state = new SessionStateHandler( $test_store );
$uniqid = uniqid();
$test_state->store( $uniqid );

$this->assertEquals( $uniqid, $test_store->get( SessionStateHandler::STATE_NAME ) );
}

public function testStateIssuedCorrectly() {
// Suppress header sent error
@$test_store = new SessionStore();
$test_state = new SessionStateHandler( $test_store );
$uniqid_returned = $test_state->issue();

$this->assertEquals( $uniqid_returned, $test_store->get( SessionStateHandler::STATE_NAME ) );
}

public function testStateValidatesCorrectly() {
// Suppress header sent error
@$test_store = new SessionStore();
$test_state = new SessionStateHandler( $test_store );
$uniqid_returned_1 = $test_state->issue();

$this->assertTrue( $test_state->validate( $uniqid_returned_1 ) );
$this->assertNull( $test_store->get( SessionStateHandler::STATE_NAME ) );

$uniqid_returned_2 = $test_state->issue();
$this->assertFalse( $test_state->validate( $uniqid_returned_2 . 'false' ) );
}
}