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

Adding pagination, tests, + docs to Client Grants; minor test suite refactor #271

Merged
merged 1 commit into from
Jul 3, 2018
Merged
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
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@
"scripts": {
"test": "SHELL_INTERACTIVE=1 vendor/bin/phpunit --colors=always --verbose ",
"test-ci": "vendor/bin/phpunit --coverage-text --coverage-clover=build/coverage.xml",
"phpcs": "SHELL_INTERACTIVE=1 ./vendor/bin/phpcs --standard=phpcs-ruleset.xml -s",
"phpcs": "./vendor/bin/phpcs --standard=phpcs-ruleset.xml -s .",
"phpcs-path": "SHELL_INTERACTIVE=1 ./vendor/bin/phpcs --standard=phpcs-ruleset.xml -s",
"phpcbf": "./vendor/bin/phpcbf --standard=phpcs-ruleset.xml .",
"phpcbf-path": "SHELL_INTERACTIVE=1 ./vendor/bin/phpcbf --standard=phpcs-ruleset.xml",
"sniffs": "./vendor/bin/phpcs --standard=phpcs-ruleset.xml -e"
Expand Down
7 changes: 6 additions & 1 deletion phpcs-ruleset.xml
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,12 @@
<exclude name="Squiz.Commenting.FileComment"/>
<exclude name="Squiz.Commenting.LongConditionClosingComment"/>
<exclude name="Squiz.Commenting.FunctionComment.ScalarTypeHintMissing"/>
<exclude name="Squiz.Commenting.FunctionCommentThrowTag.WrongNumber"/>
</rule>
<rule ref="Squiz.ControlStructures"/>
<rule ref="Squiz.Functions">
<exclude name="Squiz.Functions.MultiLineFunctionDeclaration.NewlineBeforeOpenBrace"/>
<exclude name="Squiz.ControlStructures.InlineIfDeclaration.NoBrackets"/>
</rule>
<rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing">
<properties>
Expand All @@ -75,7 +77,10 @@
<exclude name="Squiz.Operators.ComparisonOperatorUsage.NotAllowed"/>
<exclude name="Squiz.Operators.ComparisonOperatorUsage.ImplicitTrue"/>
</rule>
<rule ref="Squiz.PHP"/>
<rule ref="Squiz.PHP">
<exclude name="Squiz.PHP.DisallowComparisonAssignment.AssignedComparison"/>
<exclude name="Squiz.PHP.DisallowInlineIf.Found"/>
</rule>
<rule ref="Squiz.Scope"/>
<rule ref="Squiz.Strings"/>
<rule ref="Squiz.WhiteSpace">
Expand Down
194 changes: 156 additions & 38 deletions src/API/Management/ClientGrants.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,75 +2,193 @@

namespace Auth0\SDK\API\Management;

use Auth0\SDK\API\Header\ContentType;
use Auth0\SDK\Exception\CoreException;

/**
* Class ClientGrants.
* Handles requests to the Client Grants endpoint of the v2 Management API.
*
* @package Auth0\SDK\API\Management
*/
class ClientGrants extends GenericResource
{

/**
* Get all Client Grants, by page if desired.
* Required scope: "read:client_grants"
*
* @param array $params Additional URL parameters to send:
* - "audience" to filter be a specific API audience identifier.
* - "client_id" to return an object.
* - "include_totals" to return an object.
* @param null|integer $page The page number, zero based.
* @param null|integer $per_page The amount of entries per page.
*
* @param string $id
* @param null|string $audience
* @return mixed
*
* @throws \Exception Thrown by the Guzzle HTTP client when there is a problem with the API call.
*
* @link https://auth0.com/docs/api/management/v2#!/Client_Grants/get_client_grants
*/
public function get($id, $audience = null)
public function getAll(array $params = [], $page = null, $per_page = null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I'm changing the method name here but I'm not. The get method still exists (in a semi-broken state), moved to the bottom of the class with a TODO to deprecate.

{
$request = $this->apiClient->get()
->addPath('client-grants');
if (null !== $page) {
$params['page'] = abs(intval($page));
}

if ($audience !== null) {
$request = $request->withParam('audience', $audience);
if (null !== $per_page) {
$params['per_page'] = abs(intval($per_page));
}

return $request->call();
return $this->apiClient->method('get')
->addPath('client-grants')
->withDictParams($params)
->call();
}

/**
* Get Client Grants by audience.
* Required scope: "read:client_grants"
*
* @param string $audience API Audience to filter by.
* @param null|integer $page The page number, zero based.
* @param null|integer $per_page The amount of entries per page.
*
* @return mixed
*
* @throws CoreException Thrown when $audience is empty or not a string.
* @throws \Exception Thrown by the Guzzle HTTP client when there is a problem with the API call.
*
* @link https://auth0.com/docs/api/management/v2#!/Client_Grants/get_client_grants
*/
public function getByAudience($audience, $page = null, $per_page = null)
Copy link
Member

Choose a reason for hiding this comment

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

Is this convenience method also in other SDKs?
/cc @lbalmaceda

{
if (empty($audience) || ! is_string($audience)) {
Copy link
Member

Choose a reason for hiding this comment

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

that exclamation mark

Copy link
Member

Choose a reason for hiding this comment

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

So the rest of the code follows this, although I think it's ugly. If it's consistent with the rest of the code base, then I'm fine with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Ugly" as in you don't like the error checking here? @lbalmaceda and I talked about failing early in methods to avoid API errors. For this one, an empty $audience would not fail but it also would not filter so the method would be useless.

Copy link
Member

Choose a reason for hiding this comment

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

Don't like the space between a ! and the context. But don't have any strong feelings as all the others are like this in the PR so presume it's inline with coding style of the SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was on the fence about the space. This is all whitespace-corrected now so I can enforce no space.

throw new CoreException('Empty or invalid "audience" parameter.');
}

return $this->getAll(['audience' => $audience], $page, $per_page);
}

/**
* Get Client Grants by Client ID.
* Required scope: "read:client_grants"
*
* @param string $client_id Client ID to filter by.
* @param null|integer $page The page number, zero based.
* @param null|integer $per_page The amount of entries per page.
*
* @return mixed
*
* @throws CoreException Thrown when $client_id is empty or not a string.
* @throws \Exception Thrown by the Guzzle HTTP client when there is a problem with the API call.
*
* @link https://auth0.com/docs/api/management/v2#!/Client_Grants/get_client_grants
*/
public function getByClientId($client_id, $page = null, $per_page = null)
Copy link
Member

Choose a reason for hiding this comment

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

Is this convenience method also in other SDKs?
/cc @lbalmaceda

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I know of but I don't think that's a good reason not to add it here. Make the developer's life easier!

Copy link
Member

Choose a reason for hiding this comment

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

Just curious, it's always good to create more convenience for the end user, balanced with maintainability.

{
if (empty($client_id) || ! is_string($client_id)) {
throw new CoreException('Empty or invalid "client_id" parameter.');
}

return $this->getAll(['client_id' => $client_id], $page, $per_page);
}

/**
* Create a new Client Grant.
* Required scope: "create:client_grants"
*
* @param string $client_id Client ID to receive the grant.
* @param string $audience Audience identifier for the API being granted.
* @param array $scope Array of scopes for the grant.
*
* @return mixed
*
* @throws CoreException Thrown when $client_id or $audience are empty or not a string.
* @throws \Exception Thrown by the Guzzle HTTP client when there is a problem with the API call.
*
* @link https://auth0.com/docs/api/management/v2#!/Client_Grants/post_client_grants
*/
public function create($client_id, $audience, array $scope = [])
{
if (empty($client_id) || ! is_string($client_id)) {
throw new CoreException('Empty or invalid "client_id" parameter.');
}

if (empty($audience) || ! is_string($audience)) {
throw new CoreException('Empty or invalid "audience" parameter.');
}

return $this->apiClient->method('post')
->addPath('client-grants')
->withBody(json_encode([
'client_id' => $client_id,
'audience' => $audience,
'scope' => $scope,
]))
->call();
}

/**
* Delete a Client Grant by ID.
* Required scope: "delete:client_grants"
*
* @param string $id Client Grant ID to delete.
*
* @param string $client_id
* @param string $audience
* @param string $scope
* @return mixed
*
* @throws \Exception Thrown by the Guzzle HTTP client when there is a problem with the API call.
Copy link
Member

Choose a reason for hiding this comment

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

Although we use Guzzle I would not mention it by name, goes for all mentions in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use it in the variable name for the API. It will also appear in the error name. I think it's helpful here in case someone is getting a fatal error and want to know where to start troubleshooting.

*
* @link https://auth0.com/docs/api/management/v2#!/Client_Grants/delete_client_grants_by_id
*/
public function create($client_id, $audience, $scope)
public function delete($id)
Copy link
Member

Choose a reason for hiding this comment

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

So presume audience is no longer required? What happens in PHP if a developer makes a call to a method that has a change in signature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad diff for this one ... you can see this is a different method.

To answer your question though ... it wouldn't fail, it just wouldn't use that param.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, the one I saw was delete($id) vs delete($id, $audience = null)

{
return $this->apiClient->post()
->addPath('client-grants')
->withHeader(new ContentType('application/json'))
->withBody(json_encode([
'client_id' => $client_id,
'audience' => $audience,
'scope' => $scope,
]))
->call();
return $this->apiClient->method('delete')
->addPath('client-grants', $id)
->call();
}

/**
* Update an existing Client Grant.
* Required scope: "update:client_grants"
*
* @param string $id Client Grant ID to update.
* @param array $scope Array of scopes to update; will replace existing scopes, not merge.
*
* @param string $id
* @param null|string $audience
* @return mixed
*
* @throws \Exception Thrown by the Guzzle HTTP client when there is a problem with the API call.
*
* @link https://auth0.com/docs/api/management/v2#!/Client_Grants/patch_client_grants_by_id
*/
public function delete($id, $audience = null)
public function update($id, array $scope)
{
return $this->apiClient->delete()
->addPath('client-grants', $id)
->call();
return $this->apiClient->method('patch')
->addPath('client-grants', $id)
->withBody(json_encode(['scope' => $scope,]))
->call();
}

/**
* Get a Client Grant.
* TODO: Deprecate, cannot get a Client Grant by ID.
*
* @param string $id Client Grant ID.
* @param null|string $audience Client Grant audience to filter by.
*
* @param string $id
* @param string $scope
* @return mixed
*
* @throws \Exception Thrown by the Guzzle HTTP client when there is a problem with the API call.
*/
public function update($id, $scope)
public function get($id, $audience = null)
{
return $this->apiClient->patch()
->addPath('client-grants', $id)
->withHeader(new ContentType('application/json'))
->withBody(json_encode([
'scope' => $scope,
]))
->call();
$request = $this->apiClient->get()
->addPath('client-grants');

if ($audience !== null) {
$request = $request->withParam('audience', $audience);
}

return $request->call();
}
}
36 changes: 36 additions & 0 deletions tests/API/ApiTests.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,18 @@
namespace Auth0\Tests\API;

use Auth0\SDK\API\Helpers\TokenGenerator;
use Auth0\SDK\API\Management;
use josegonzalez\Dotenv\Loader;

class ApiTests extends \PHPUnit_Framework_TestCase
{

/**
*
* @var array
*/
protected static $env = [];

protected function getEnv()
{
return self::getEnvStatic();
Expand Down Expand Up @@ -46,4 +53,33 @@ protected static function getTokenStatic($env, $scopes)
);
return $generator->generate($scopes);
}

/**
* Return an API client used during self::setUpBeforeClass().
*
* @param string $endpoint Endpoint name used for token generation.
* @param array $actions Actions required for token generation.
*
* @return mixed
*/
protected static function getApiStatic($endpoint, array $actions)
{
self::$env = self::getEnvStatic();
$token = self::getTokenStatic(self::$env, [$endpoint => ['actions' => $actions]]);
$api_client = new Management($token, self::$env['DOMAIN']);
return $api_client->$endpoint;
}

/**
* Does an error message contain a specific string?
*
* @param \Exception $e - Error object.
* @param string $str - String to find in the error message.
*
* @return boolean
*/
protected function errorHasString(\Exception $e, $str)
{
return ! (false === strpos($e->getMessage(), $str));
}
}
24 changes: 2 additions & 22 deletions tests/API/BasicCrudTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,6 @@ abstract class BasicCrudTest extends ApiTests
*/
protected $domain;

/**
* Environment variables, generated in self::__construct().
*
* @var array
*/
protected $env;

/**
* API client to test.
*
Expand Down Expand Up @@ -93,8 +86,8 @@ abstract protected function afterUpdate($updated_entity);
public function __construct()
{
parent::__construct();
$this->env = $this->getEnv();
$this->domain = $this->env['DOMAIN'];
self::$env = $this->getEnv();
$this->domain = self::$env['DOMAIN'];
$this->api = $this->getApiClient();
$this->rand = rand();
}
Expand Down Expand Up @@ -122,19 +115,6 @@ protected function getId($entity)
return $entity[$this->id_name];
}

/**
* Does an error message contain a specific string?
*
* @param \Exception $e - Error object.
* @param string $str - String to find in the error message.
*
* @return boolean
*/
protected function errorHasString(\Exception $e, $str)
{
return ! (false === strpos($e->getMessage(), $str));
}

/**
* Check that HTTP options have been set correctly.
*/
Expand Down
2 changes: 1 addition & 1 deletion tests/API/Management/AuthApiTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public function testOauthToken()

$token = $api->client_credentials(
[
'audience' => 'tests'
'audience' => 'https://'.$env['DOMAIN'].'/api/v2/'
]
);

Expand Down
Loading