Adding refresh token support; adjusting default scope #456

joshcanhelp · 2018-05-04T13:20:20Z

Adds refresh token support. Developers would add an offline_access key via the auth0_auth_token_scope filter, then store that token in the auth0_user_login action.

Also adds a profile scope to address some missing data in implicit and the /userinfo fallback. Since this PR is addressing scope and I found it while testing this, I added it here even though it's not directly related.

Closes #296

lbalmaceda

Keeping in mind that refresh tokens can't be revoked, how safe is this storage?
I find sort of confusing the auth0_auth_token_scope name, but I guess it can't be changed now? Can you please share a small snippet on how would a user pass this customized value?

lbalmaceda · 2018-05-07T17:27:18Z

lib/WP_Auth0_LoginManager.php

@@ -232,9 +232,13 @@ public function redirect_login() {
 			throw new WP_Auth0_LoginFlowValidationException( $e_message, $e_code );
 		}

+		$access_token = $data->access_token;
+		$id_token = $data->id_token;


how can you be so sure that the id_token is present?? you might want to null check like you do for refresh tokens.

Why wouldn't it be? If I'm requesting it then it should be there if the access token is there, right? Is there a case where I would request both and the id_token is missing?

only if the response_type includes id_token (i.e. token id_token) the id_token should be present. At least that's how it's supposed to work :D

Well, this being more of an app than an SDK, that's something the plugin controls and doesn't change. Both flows currently get an id_token and have no reason to change.

Reading this again after yesterday's discussion, this method handles code exchange redirection so here we have access_token and id_token. No need to null-check them 👍

I confirmed that OIDC and non-OIDC applications both prove access_token and id_token with response_type=code

lbalmaceda · 2018-05-07T17:28:02Z

lib/WP_Auth0_LoginManager.php

+		$access_token = $data->access_token;
+		$id_token = $data->id_token;
+		$refresh_token = isset( $data->refresh_token ) ? $data->refresh_token : null;
+
 		// Decode the incoming ID token for the Auth0 user.
 		$decoded_token = JWT::decode(


does this work / would it make sense for this to be called if the token is missing?

My point as well ... we need it for this. I guess we could check for both but seems duplicative.

What do you mean by both? decode function is only using id_token. And btw access_token is always present.

Sorry ... both access_token and id_token. They're both returned at the same time so if we have an access_token, we also have an id_token.

lbalmaceda · 2018-05-07T17:35:52Z

lib/WP_Auth0_LoginManager.php

 	 *
 	 * @return bool
 	 *
 	 * @throws WP_Auth0_LoginFlowValidationException - OAuth login flow errors.
 	 * @throws WP_Auth0_BeforeLoginException - Errors encountered during the auth0_before_login action.
 	 */
-	public function login_user( $userinfo, $id_token, $access_token ) {
+	public function login_user( $userinfo, $id_token = null, $access_token = null, $refresh_token = null ) {


Since you're not using the values but passing them to the do_login function, shouldn't these defaults go in that function?

This is the public method, do_login is private. We don't need to call login_user with values here so I don't want to require them.

lbalmaceda · 2018-05-07T17:36:04Z

lib/WP_Auth0_LoginManager.php

-	 * @param string $id_token - user's ID token returned from Auth0.
-	 * @param string $access_token - user's access token returned from Auth0; not provided when using implicit_login().
+	 * @param null|string $id_token - user's ID token returned from Auth0.
+	 * @param null|string $access_token - user's access token returned from Auth0; not provided when using implicit.


not provided when using implicit.

how not?

We don't request one and don't need one. We're not pinging back to Auth0 in this case, which is the purpose of implicit, the WP server cannot connect to auth0.com for whatever reason.

I get it that implicit doesn't receive an access_token since you're not requesting it. Fine. But This method doesn't know if the user called implicit flow or code flow. So, the description of the method should just say that null values are accepted for access_token and refresh_token, just that. And if you want, in the method that handles the implicit result parsing you can add in the docblock that you'll never receive an access_token or refresh_token there.

joshcanhelp · 2018-05-07T21:04:01Z

Keeping in mind that refresh tokens can't be revoked, how safe is this storage?

There is no storage here, just the ability to request and the ability to process.

I find sort of confusing the auth0_auth_token_scope name, but I guess it can't be changed now?

It can, it was introduced for this release ... I'm open to suggestions.

Can you please share a small snippet on how would a user pass this customized value?

This will be added to the plugin extension docs after release but, in short:

function auth0_theme_hook_auth0_auth_token_scope( $scope ) {
	$scope[] = 'offline_access';
	return $scope;
}
add_filter( 'auth0_auth_token_scope', 'auth0_theme_hook_auth0_auth_token_scope' );

☝️ that's how one would add the refresh_token request.

function auth0_theme_hook_auth0_user_login( $user_id, $userinfo, $is_new, $id_token, $access_token, $refresh_token ) {
	echo $refresh_token ? $refresh_token : 'not provided';
        die();
}
add_action( 'auth0_user_login', 'auth0_theme_hook_auth0_user_login', 10, 6 );

☝️ that's how you would do something with the refresh token. That action runs right after a user is authenticated in WP.

lbalmaceda · 2018-05-08T13:51:02Z

lib/WP_Auth0_LoginManager.php

@@ -333,7 +337,7 @@ public function implicit_login() {
 		// Populate legacy userinfo property.
 		$decoded_token->user_id = $decoded_token->sub;

-		if ( $this->login_user( $decoded_token, $token, null ) ) {
+		if ( $this->login_user( $decoded_token, $token ) ) {


Can the $_POST['token'] "token" param be renamed before next release? And please rename $token to $id_token or similar, it's easier to follow up which token is it.

https://github.com/auth0/wp-auth0/blob/6382a6ca84c1be0ff64a80a4dbfbfc5efeaead39/lib/WP_Auth0_LoginManager.php#L304

The key in $_POST cannot, unfortunately. That's the name it's had for a while so changing would be breaking. Just changed the internal var name, good call.

lbalmaceda · 2018-05-08T14:10:01Z

lib/WP_Auth0_LoginManager.php

 	 *
 	 * @throws WP_Auth0_BeforeLoginException - Errors encountered during the auth0_before_login action.
 	 */
-	private function do_login( $user, $userinfo, $is_new, $id_token, $access_token ) {
+	private function do_login( $user, $userinfo, $is_new, $id_token, $access_token, $refresh_token ) {


what's the difference between login_user and do_login methods? can these methods be renamed?

login_user is called after auth is successful so it decides if someone can login or if we need to make an account. do_login is called if login_user is successful, handles core WP login and hooks.

can these get renamed to something more clear?

i.e.:

login_user is processing an authentication and deciding what to do next. Shouldn't that be something like process_successful_auth?

and do_login well, it's more straight forward. But it could be do_wp_login

lbalmaceda · 2018-05-08T14:10:11Z

lib/WP_Auth0_LoginManager.php

@@ -462,10 +467,11 @@ public function login_user( $userinfo, $id_token, $access_token ) {
 	 * @param bool   $is_new - `true` if the user was created in the WordPress database, `false` if not.
 	 * @param string $id_token - user's ID token returned from Auth0.
 	 * @param string $access_token - user's access token returned from Auth0; not provided when using implicit_login().


same docblock comment

lbalmaceda · 2018-05-08T14:13:22Z

About auth0_auth_token_scope I'd just call it auth0_auth_scope or auth0_authentication_scope. The the scope is added to the tokens (plural), but in all the platforms I recall I saw that as authentication scope or just "scope" param that you called setScope or addAuthenticationParams({dic with scope param})

lbalmaceda

LGTM 🌮

joshcanhelp added CH: Added labels May 4, 2018

joshcanhelp added this to the 3.6.0 milestone May 4, 2018

lbalmaceda reviewed May 7, 2018

View reviewed changes

lbalmaceda reviewed May 8, 2018

View reviewed changes

Adding refresh token support; adjusting default scope

d99db35

joshcanhelp force-pushed the add-refresh-token-support branch from 3716c59 to d99db35 Compare May 8, 2018 16:58

lbalmaceda approved these changes May 9, 2018

View reviewed changes

joshcanhelp merged commit 8b3b515 into dev May 9, 2018

joshcanhelp deleted the add-refresh-token-support branch May 9, 2018 15:44

joshcanhelp removed the Ready for Review label May 9, 2018

joshcanhelp mentioned this pull request May 24, 2018

Is there a way to use Refresh Tokens and Wordpress? #296

Closed

joshcanhelp mentioned this pull request Jun 5, 2018

Release 3.6.0 #475

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding refresh token support; adjusting default scope #456

Adding refresh token support; adjusting default scope #456

joshcanhelp commented May 4, 2018 •

edited

lbalmaceda left a comment

lbalmaceda May 7, 2018

joshcanhelp May 7, 2018

lbalmaceda May 7, 2018

joshcanhelp May 7, 2018

lbalmaceda May 8, 2018

joshcanhelp May 8, 2018

lbalmaceda May 7, 2018

joshcanhelp May 7, 2018

lbalmaceda May 7, 2018 •

edited

joshcanhelp May 7, 2018

lbalmaceda May 7, 2018

joshcanhelp May 7, 2018

lbalmaceda May 7, 2018

joshcanhelp May 7, 2018

lbalmaceda May 8, 2018

joshcanhelp commented May 7, 2018 •

edited

lbalmaceda May 8, 2018

joshcanhelp May 8, 2018

lbalmaceda May 8, 2018 •

edited

joshcanhelp May 8, 2018

lbalmaceda May 9, 2018

lbalmaceda May 8, 2018

lbalmaceda commented May 8, 2018

lbalmaceda left a comment

Adding refresh token support; adjusting default scope #456

Adding refresh token support; adjusting default scope #456

Conversation

joshcanhelp commented May 4, 2018 • edited

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda May 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp commented May 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda May 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda commented May 8, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

joshcanhelp commented May 4, 2018 •

edited

lbalmaceda May 7, 2018 •

edited

joshcanhelp commented May 7, 2018 •

edited

lbalmaceda May 8, 2018 •

edited