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.

Sign up for GitHub

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

Jump to bottom

Adding refresh token support; adjusting default scope #456

Merged
merged 1 commit into from
May 9, 2018
Merged

Adding refresh token support; adjusting default scope #456

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 23 additions & 19 deletions lib/WP_Auth0_LoginManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

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 confirmed that OIDC and non-OIDC applications both prove access_token and id_token with response_type=code

$refresh_token = isset( $data->refresh_token ) ? $data->refresh_token : null;

// Decode the incoming ID token for the Auth0 user.
$decoded_token = JWT::decode(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@lbalmaceda lbalmaceda May 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

$data->id_token,
$id_token,
$this->a0_options->get_client_secret_as_key(),
array( $this->a0_options->get_client_signing_algorithm() )
);
Expand All @@ -251,7 +255,7 @@ public function redirect_login() {
// Management API call failed, fallback to userinfo.
if ( 200 !== $userinfo_resp_code || empty( $userinfo_resp_body ) ) {

$userinfo_resp = WP_Auth0_Api_Client::get_user_info( $domain, $data->access_token );
$userinfo_resp = WP_Auth0_Api_Client::get_user_info( $domain, $access_token );
$userinfo_resp_code = (int) wp_remote_retrieve_response_code( $userinfo_resp );
$userinfo_resp_body = wp_remote_retrieve_body( $userinfo_resp );

Expand All @@ -267,7 +271,7 @@ public function redirect_login() {

$userinfo = json_decode( $userinfo_resp_body );

if ( $this->login_user( $userinfo, $data->id_token, $data->access_token ) ) {
if ( $this->login_user( $userinfo, $id_token, $access_token, $refresh_token ) ) {
$state_decoded = $this->get_state( true );
if ( ! empty( $state_decoded->interim ) ) {
include WPA0_PLUGIN_DIR . 'templates/login-interim.php';
Expand All @@ -293,20 +297,18 @@ public function redirect_login() {
* @link https://auth0.com/docs/api-auth/tutorials/implicit-grant
*
* @see /wp-content/plugins/auth0/assets/js/implicit-login.js
*
* TODO: Add a WP nonce!
*/
public function implicit_login() {
if ( empty( $_POST['token'] ) ) {
throw new WP_Auth0_LoginFlowValidationException( __( 'No ID token found', 'wp-auth0' ) );
}

// Posted from the login page to the callback URL.
$token = sanitize_text_field( wp_unslash( $_POST['token'] ) );
$id_token = sanitize_text_field( wp_unslash( $_POST['token'] ) );

try {
$decoded_token = JWT::decode(
$token,
$id_token,
$this->a0_options->get_client_secret_as_key(),
array( $this->a0_options->get_client_signing_algorithm() )
);
Expand All @@ -333,7 +335,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, $id_token ) ) {

// Validated above in $this->init_auth0().
$state_decoded = $this->get_state( true );
Expand All @@ -355,15 +357,16 @@ public function implicit_login() {
* Attempts to log the user in and create a new user, if possible/needed.
*
* @param object $userinfo - Auth0 profile of the user.
* @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 if returned from Auth0.
* @param null|string $access_token - user's access token if returned from Auth0.
* @param null|string $refresh_token - user's refresh token if returned from Auth0.
*
* @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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// Check that the user has a verified email, if required.
if ( ! $this->ignore_unverified_email && $this->a0_options->get( 'requires_verified_email' ) ) {
if ( empty( $userinfo->email ) ) {
Expand Down Expand Up @@ -420,7 +423,7 @@ public function login_user( $userinfo, $id_token, $access_token ) {

$this->users_repo->update_auth0_object( $user->data->ID, $userinfo );
$user = apply_filters( 'auth0_get_wp_user', $user, $userinfo );
$this->do_login( $user, $userinfo, false, $id_token, $access_token );
$this->do_login( $user, $userinfo, false, $id_token, $access_token, $refresh_token );
return true;
} else {

Expand All @@ -435,7 +438,7 @@ public function login_user( $userinfo, $id_token, $access_token ) {
$this->ignore_unverified_email
);
$user = get_user_by( 'id', $user_id );
$this->do_login( $user, $userinfo, true, $id_token, $access_token );
$this->do_login( $user, $userinfo, true, $id_token, $access_token, $refresh_token );
} catch ( WP_Auth0_CouldNotCreateUserException $e ) {

throw new WP_Auth0_LoginFlowValidationException( $e->getMessage() );
Expand All @@ -460,12 +463,13 @@ public function login_user( $userinfo, $id_token, $access_token ) {
* @param object $user - the WP user object, such as returned by get_user_by().
* @param object $userinfo - the Auth0 profile of the user.
* @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().
* @param null|string $id_token - user's ID token if returned from Auth0, otherwise null.
* @param null|string $access_token - user's access token if returned from Auth0, otherwise null.
* @param null|string $refresh_token - user's refresh token if returned from Auth0, otherwise null.
*
* @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 ) {
Copy link
Contributor

@lbalmaceda lbalmaceda May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

$remember_users_session = $this->a0_options->get( 'remember_users_session' );

try {
Expand All @@ -487,7 +491,7 @@ private function do_login( $user, $userinfo, $is_new, $id_token, $access_token )

wp_set_auth_cookie( $user->ID, $remember_users_session, $secure_cookie );
do_action( 'wp_login', $user->user_login, $user );
do_action( 'auth0_user_login', $user->ID, $userinfo, $is_new, $id_token, $access_token );
do_action( 'auth0_user_login', $user->ID, $userinfo, $is_new, $id_token, $access_token, $refresh_token );
}

/**
Expand Down Expand Up @@ -594,8 +598,8 @@ public function end_session() {
* @return string
*/
public static function get_userinfo_scope( $context = '' ) {
$default_scope = array( 'openid', 'email', 'name', 'nickname', 'picture' );
$filtered_scope = apply_filters( 'auth0_auth_token_scope', $default_scope, $context );
$default_scope = array( 'openid', 'email', 'profile' );
$filtered_scope = apply_filters( 'auth0_auth_scope', $default_scope, $context );
return implode( ' ', $filtered_scope );
}

Expand Down