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

Creating client_grant for management API #366

Merged
merged 1 commit into from
Jan 23, 2018
Merged

Creating client_grant for management API #366

merged 1 commit into from
Jan 23, 2018

Conversation

joshcanhelp
Copy link
Contributor

Clients used with this plugin need to have a specific grant to the management API in order to handle certain actions like verification emails. The client also needs to have an app_type set, in this case "Regular Web Application."

This pull request:

  • Adds the app_type and client_grant to the client creation process when the plugin is first installed
  • Updates the DB version to 16 and attempts to fix the app_type and client_grant
  • If this process fails, a flag is set as an option and a banner with instructions will appear until the process is complete
  • Added a process to extra the audience from a manually-added app token during validation
  • Added a check for the client grant when an app token is saved to re-run the client_grant process
  • A few spelling and formatting errors throughout

@joshcanhelp joshcanhelp requested a review from cocojoe January 19, 2018 17:32
@joshcanhelp joshcanhelp added this to the v3-Next milestone Jan 19, 2018
@cocojoe
Copy link
Member

cocojoe commented Jan 20, 2018

@joshcanhelp for these bigger commits, the formatting type corrections just add unnecessary noise to the review. I'm all for improving readability and consistent. However, in the future these would be better in a separate PR.

@cocojoe
Copy link
Member

cocojoe commented Jan 20, 2018

Please rebase and a good time to try the mergetool 😄

@joshcanhelp
Creating a new client grant on install and adding the app_type for th…
e6c9d84 
…e client created; DB version upgrade to 16 and resulting changes needed (client grant and app_type update); notice and flag for failed client grant addition; set app_token_audience and check for client grant flag when saving a new app token; helper functions for converting client_id to a key
@joshcanhelp joshcanhelp force-pushed the fixed-client-grant-to-management-api branch from 7ce0eac to e6c9d84 Compare January 20, 2018 21:55
@joshcanhelp
Copy link
Contributor Author

@cocojoe - GTG

cocojoe
cocojoe reviewed Jan 22, 2018
View reviewed changes
Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

@joshcanhelp it would be great if you could provide some screenshots of the upgrade process. Thanks

lib/WP_Auth0_DBManager.php
if ( !empty( $app_token ) &&
!empty( $connection_id ) &&
!empty( $migration_token ) ) {

$response = $operations->update_wordpress_connection(
$operations->update_wordpress_connection(
Copy link
Member

Choose a reason for hiding this comment

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

$response never used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope!

lib/WP_Auth0_DBManager.php
} else {
$options->set('use_lock_10', true);
}
if ( strpos( $cdn_url, '10.' ) === false ) {
Copy link
Member

Choose a reason for hiding this comment

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

I mentioned this but leaving it for other reviewers, in the future let's cover these changes in a separate internal PR, to keep the review process cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cocojoe - Understood. Can all formatting and minor stuff go together in a final cleanup PR or something?

Most of the cases where there was formatting changes were part of a minor refactor. In this case, I moved all the vars we were using to the top of that method and reformatted a couple of the blocks.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's not a priority. However, if you have some time and want to do a lint pass and also adopt some linting standards then it would be good to do. Although more likely this will be for the vNext when this can be standardised from the start.

lib/WP_Auth0_LoginManager.php
trigger_error( __( 'Method dieWithVerifyEmail is deprecated.', 'wp-auth0' ), E_USER_DEPRECATED);
WP_Auth0_Email_Verification::render_die( $userinfo );
}

Copy link
Member

Choose a reason for hiding this comment

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

Separate PR in future.

lib/admin/WP_Auth0_Admin_Basic.php
$error = __( 'You need to specify domain', 'wp-auth0' );
$this->add_validation_error( $error );
$completeBasicData = false;
$this->add_validation_error( __( 'You need to specify domain', 'wp-auth0' ) );
Copy link
Member

Choose a reason for hiding this comment

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

This is nice

@cocojoe cocojoe removed the CH: Added label Jan 22, 2018
@joshcanhelp
Copy link
Contributor Author

joshcanhelp commented Jan 22, 2018 •
edited
Loading

@cocojoe - If the upgrade works without manual intervention, you won't see anything at all. But, if not, you'll see this message at the top of all screens if you're an admin (update_plugins capability):

screenshot 2018-01-22 07 26 21

You can simulate this on your local install by turning off the Non-Interactive Client created by WP in the Auth0 Management API:

screenshot 2018-01-22 09 33 00

... and adding an option:

update_option( 'wp_auth0_client_grant_failed', 1 );

@joshcanhelp joshcanhelp mentioned this pull request Jan 22, 2018
Merged
@cocojoe cocojoe requested a review from glena January 23, 2018 09:43
@glena
Copy link
Contributor

glena commented Jan 23, 2018

I think it looks good, I would add a patch to the client to set the grant_types. For wegular webapp you will only need code and client_credentials. If implicit is enabled, you should add implicit and remove code

@joshcanhelp
Copy link
Contributor Author

@glena - Looks like the clients we're creating have the three grant_types you mentioned, as well as "Refresh Token" by default. That's the case when you create a new one in the dashboard as well.

@joshcanhelp joshcanhelp merged commit 0a9313e into dev Jan 23, 2018
@joshcanhelp joshcanhelp deleted the fixed-client-grant-to-management-api branch January 23, 2018 15:02
@cocojoe cocojoe mentioned this pull request Jan 25, 2018
Merged
@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.
Reviewers

@cocojoe cocojoe cocojoe left review comments

@glena glena Awaiting requested review from glena

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@joshcanhelp @cocojoe @glena