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

Refactor Token Manager / Auth / Token Connecting/Disconnecting #77

Merged

Conversation

defunctl
Copy link
Contributor

@defunctl defunctl commented Jul 17, 2024

Main Changes

This PR brings in some modified changes from #51, sort of a half way of that entire PR that Kadence is currently using live.

NOTE: I spoke to @borkweb and only Kadence is using tokens right now, so any backwards compatibility is for that product, however Kadence's multisite licensing management is totally different than TEC, something we'll have to sort out down the road. This PR gets us partially there.

Token Manager

  • This now takes a Resource instance for relevant methods, except delete() which still takes a slug in the event developers need to clean things up after a Resource is gone. Note, that disconnecting won't delete a resource that doesn't exist.
  • The get() method will fall back to getting the legacy token if no specific key for a slug is found.
  • Using get_all() with a stored legacy token will include a [ 'legacy' => 'the token here' ] entry in the array, essentially legacy becomes the product slug.
  • Running delete() on any slug will always delete the legacy token if it exists.

Token Connecting/Disconnecting

  • This now fires on a unique action attached to the Config Prefix and the Resource slug to properly handle sites that have multiple plugins with the Uplink library, so they don't attempt to connect/disconnect each other's tokens.

API

Authorizer

  • This now just checks if a user is authorized to connect/disconnect tokens, which is filterable and defaults to is_super_admin(). If you actually want to see if a user has a valid remote license token, use \StellarWP\Uplink\is_authorized(). If you want to manually check if a user is authorized to perform token management, use \StellarWP\Uplink\is_user_authorized().

Admin Notices

  • Admin notices now allow some limited HTML tags in their message, where they were previously escaped before.

New Functions

  • There are lots of new functions in functions.php that abstract away some of the complexity. This should be the preferred way to use this library vs calling the Config object all over the place of their plugin. This way we have a single interface to update, and for plugin developers as well in the case underlying objects need to change down the road, often that means the function arguments don't have to change.

@defunctl defunctl self-assigned this Jul 17, 2024
@defunctl defunctl changed the title Refactor Token Manager to use a Resource Refactor Token Manager Jul 18, 2024
@defunctl defunctl marked this pull request as ready for review July 18, 2024 02:48
@defunctl defunctl changed the title Refactor Token Manager Refactor Token Manager / Auth / Token Connecting/Disconnecting Jul 18, 2024
Copy link
Member

@dpanta94 dpanta94 left a comment

Choose a reason for hiding this comment

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

Looks really good and thank you so much for this! Left some comments/notes

src/Uplink/Auth/Action_Manager.php Outdated Show resolved Hide resolved
Comment on lines +97 to +103
$this->notice->add( new Notice( Notice::ERROR,
sprintf(
__( 'The Litespeed plugin was detected, ensure "Store Transients" is set to ON and try again. See the <a href="%s" target="_blank">Litespeed documentation</a> for more information.', '%TEXTDOMAIN%' ),
esc_url( 'https://docs.litespeedtech.com/lscache/lscwp/cache/#store-transients' )
),
true
) );
Copy link
Member

Choose a reason for hiding this comment

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

I think we should switch to option for our nonce verification instead of transients. Have talked with @bordoni and unfortunately we are aware of customers completely disabling transients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed on slack, we'll open a new PR after this one to start working on that.


if ( $result ) {
// Delete the authorization cache.
delete_transient( Token_Authorizer_Cache_Decorator::TRANSIENT_PREFIX . $cache_key );
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned above, i'd love to get your opinion about migrating transient usage to options instead.

src/Uplink/Auth/Token/Token_Manager.php Show resolved Hide resolved
src/Uplink/Auth/Token/Token_Manager.php Show resolved Hide resolved
src/Uplink/functions.php Outdated Show resolved Hide resolved
src/Uplink/API/V3/Auth/Token_Authorizer.php Show resolved Hide resolved
src/Uplink/Auth/Admin/Connect_Controller.php Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

These new functions are great, good thinking!

tests/wpunit/Auth/Token/MultipleTokensTest.php Outdated Show resolved Hide resolved
@defunctl defunctl requested review from dpanta94 and tarecord July 18, 2024 15:36
@defunctl defunctl merged commit c774714 into feat/support-multiple-tokens Jul 18, 2024
13 checks passed
@defunctl defunctl deleted the feat/use-resource-for-token-manager branch July 18, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants