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

Get new access token via refresh token API #730

Merged
merged 4 commits into from
Nov 20, 2019
Merged

Conversation

albeja
Copy link
Contributor

@albeja albeja commented Oct 8, 2019

Added a class for requesting a new access token via the refresh token (as old "get_token()"-method was deprecated and deleted lately).

Followed the coding style from WP_Auth0_Api_Exchange_Code.php.

@albeja albeja changed the title Add files via upload Get new access token via refresh token API Oct 8, 2019
@joshcanhelp
Copy link
Contributor

@albeja - Thanks for this! I'll review as soon as I have a chance.

@damieng damieng requested a review from a team October 8, 2019 22:10
@joshcanhelp joshcanhelp requested review from joshcanhelp and removed request for a team October 8, 2019 22:35
Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

Thanks again for this! A few changes to make are detailed above. If you're able to add tests as well, that would be helpful but I see that the exchange code class is untested so you don't have a great example to use.

lib/api/WP_Auth0_Api_Refresh_Access_Token.php Outdated Show resolved Hide resolved
lib/api/WP_Auth0_Api_Refresh_Access_Token.php Show resolved Hide resolved
lib/api/WP_Auth0_Api_Refresh_Access_Token.php Show resolved Hide resolved
/**
* Make the API call and handle the response.
*
* @param string|null $code - Authorization code to exchange for tokens.
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be updated with actual method params

->add_body( 'refresh_token', $refresh_token )
->post()
->handle_response( __METHOD__ );

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

WP_Auth0_ErrorManager::insert_auth0_error(
__METHOD__ . ' L:' . __LINE__,
__( 'An /oauth/token call triggered a 401 response from Auth0. ', 'wp-auth0' ) .
__( 'Please check the Client Secret saved in the Auth0 plugin settings. ', 'wp-auth0' )
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
__( 'Please check the Client Secret saved in the Auth0 plugin settings. ', 'wp-auth0' )
__( 'Please check the Client ID and Client Secret saved in the Auth0 plugin settings. ', 'wp-auth0' )

lib/api/WP_Auth0_Api_Refresh_Access_Token.php Show resolved Hide resolved
Implemented minor changes from Josh's review.

Co-Authored-By: Josh Cunningham <josh@joshcanhelp.com>
@joshcanhelp joshcanhelp requested review from joshcanhelp and removed request for a team October 14, 2019 15:02
Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

Just one last suggestion and I think we're good here. I don't mind handling the tests for this, along with the auth code exchange as well.

If you have any working code samples for using this, though, that would be helpful to have in the PR description.

Thank you once again!

lib/api/WP_Auth0_Api_Refresh_Access_Token.php Outdated Show resolved Hide resolved
@joshcanhelp joshcanhelp added this to the 4.0.0 milestone Oct 14, 2019
Co-Authored-By: Josh Cunningham <josh@joshcanhelp.com>
@albeja
Copy link
Contributor Author

albeja commented Oct 15, 2019

Wonderful! Unfortunately I have difficulties with my local phpunit-environment at the moment. That's why I did not write any tests so far.
I'll try to fix that issue this week so that I'm able to contribute to the tests as well.

@joshcanhelp
Copy link
Contributor

@albeja - I'll leave this open for now so you can add those tests. Let me know if you need any guidance setting up the testing suite beyond the docs we have.

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

Forgot this was still open. Thanks again for the contribution @albeja!

@joshcanhelp joshcanhelp merged commit 359bb7d into auth0:master Nov 20, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants