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

LICENS-45 Add auth logic #16

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
19 changes: 17 additions & 2 deletions src/Uplink/API/Validation_Response.php
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ public function handle_api_errors() : stdClass {
'id',
'slug',
'version',
'auth_required',
'homepage',
'download_url',
'upgrade_notice',
Expand Down Expand Up @@ -492,8 +493,22 @@ public function to_wp_format() {
}
}

//Other fields need to be renamed and/or transformed.
$info->download_link = isset( $this->response->download_url ) ? $this->response->download_url . '&pu_get_download=1' : '';
if ( empty( $this->response->auth_required ) || $this->resource->has_valid_auth_token( (array) $this->response->origin ) ) {
$info->download_link = isset($this->response->download_url) ? $this->response->download_url . '&pu_get_download=1' : '';
} else {
$url = $this->response->origin->url;
Copy link
Contributor

Choose a reason for hiding this comment

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

Some alignment inconsistencies here

$query_params = [
'callback_uri' => urlencode( sprintf( '%s/stellarwp/connect', get_site_url() ) ),
'refer' => urlencode( wp_get_referer() ),
];
$url = sprintf( '%s/stellarwp/oauth_connect/login?%s', $url, http_build_query( $query_params ) );
Copy link
Member

Choose a reason for hiding this comment

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

This endpoint will need to be namespaced with the plugin/plugin suite that is implementing this library. We have to assume that multiple plugins will be installed and active on a given site with this same library in use.


$info->api_upgrade = sprintf(
esc_html__( 'Please connect plugin on Setting page in order to receive updates. You can use plugin settings page or follow this <a href="%s">link</a>', '%TEXTDOMAIN%' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a bit strange to not provide a link to the settings page. Should we just recommend they use the oauth_connect link and avoid referencing the plugin settings since implementations will be unique for each plugin? Here's what that could look like:

Suggested change
esc_html__( 'Please connect plugin on Setting page in order to receive updates. You can use plugin settings page or follow this <a href="%s">link</a>', '%TEXTDOMAIN%' ),
esc_html__( 'Please <a href="%s">authenticate this plugin</a> to receive updates.', '%TEXTDOMAIN%' ),

$url
);
$info->download_link = '';
}

if ( ! empty( $this->author_homepage ) && ! empty( $this->response->author ) ) {
$info->author = sprintf( '<a href="%s">%s</a>', esc_url( $this->author_homepage ), $this->response->author );
Expand Down
95 changes: 95 additions & 0 deletions src/Uplink/Admin/Actions.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
<?php declare(strict_types=1);

namespace StellarWP\Uplink\Admin;

use StellarWP\Uplink\API;
use StellarWP\Uplink\Config;
use StellarWP\Uplink\Resources\Collection;
use StellarWP\Uplink\Site\Data;

class Actions {

const QUERY_VAR = 'stellarwp_action';

/**
* @return void
* @action init
*/
public function register_route() {
add_rewrite_endpoint( 'stellarwp', EP_ROOT, self::QUERY_VAR );
}

/**
* @param \WP $wp
*
* @return void
* @action parse_request
*/
public function handle_auth_request( $wp ) {
if ( empty( $wp->query_vars[ self::QUERY_VAR ] ) ) {
return;
}

$args = explode( '/', $wp->query_vars[ self::QUERY_VAR ] );

if ( ! empty( $args['disconnect'] ) ) { // @phpstan-ignore-line
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the error you are ignoring here?

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 for Offset 'disconnect' on non-empty-array<int, string> in isset() does not exist.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do an apply_filters for the args before this if statement.

$this->handle_disconnect();
}

$this->handle_connect( $args );
}

/**
* Remove auth token§
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
* Remove auth token§
* Remove auth tokens

*/
public function handle_disconnect() {
$license = $this->get_license_object();

delete_option( sprintf( 'stellarwp_origin_%s_auth_token', $license->origin->slug ?? '' ) );
Copy link
Member

Choose a reason for hiding this comment

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

The option needs to be implementation-namespaced.


wp_safe_redirect( wp_get_referer() );
Copy link
Member

Choose a reason for hiding this comment

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

We should fire an action before the redirect.

exit();
}

public function handle_connect( $args ) {
if ( empty( $args['token'] ) ) {
$url = $this->get_origin_url();
if ( empty( $url ) ) {
return;
}

$query_params = [
'callback_uri' => urlencode( sprintf( '%s/stellarwp/connect', get_site_url() ) ),
Copy link
Member

Choose a reason for hiding this comment

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

This URL needs to be implementation-namespaced.

'refer' => urlencode( wp_get_referer() ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Alignment here

];
$url = sprintf( '%s/stellarwp/oauth_connect/login?%s', $url, http_build_query( $query_params ) );

wp_safe_redirect( $url );
Copy link
Member

Choose a reason for hiding this comment

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

We should fire an action before the redirect.

exit();
}

Config::get_container()->get( Data::class )->save_auth_token( $args['token'] );

wp_safe_redirect( $args['refer'] );
exit();
}

protected function get_origin_url() {
$license = $this->get_license_object();
$api = Config::get_container()->get( API\Client::class );
$origin = $api->post('/origin', [ 'slug' => $license->get_slug() ] );

if ( ! empty( $origin ) ) {
return $origin->url . '/stellarwp_connect';
}

return '';
}

protected function get_license_object() {
$collection = Config::get_container()->get( Collection::class );
$plugin = $collection->current();

return $plugin->get_license_object();
}
}
34 changes: 34 additions & 0 deletions src/Uplink/Admin/Auth.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php declare(strict_types=1);

namespace StellarWP\Uplink\Admin;

use StellarWP\Uplink\Config;
use StellarWP\Uplink\Resources\Collection;

class Auth {

public function do_auth_html() {
$collection = Config::get_container()->get( Collection::class );
$plugin = $collection->current();
$license = $plugin->get_license_object();

$token = get_option( sprintf( 'stellarwp_origin_%s_auth_token', $license->origin->slug ?? '' ), '' );
Copy link
Member

Choose a reason for hiding this comment

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

The option needs to be implementation-namespaced.

$message = esc_html__( 'Connect to origin', '%TEXTDOMAIN%' );
Copy link
Member

Choose a reason for hiding this comment

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

The customer won't know what origin means - we should avoid using that terminology for customer-facing text. Perhaps we output the Origin Site's name instead?

$classes = [ 'button', 'button-primary' ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Defining the structure here looks too prescriptive for the plugins implementing this. Could we provide a new filter to allow them to override the HTML?

$url = '/stellarwp/connect';

if ( ! empty( $token ) ) {
$message = esc_html__( 'Disconnect from origin', '%TEXTDOMAIN%' );
Copy link
Member

Choose a reason for hiding this comment

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

Need to retire the origin terminology from the text.

$classes = [ 'button', 'button-secondary'];
$url = '/stellarwp/disconnect';
Copy link
Member

Choose a reason for hiding this comment

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

The option needs to be implementation-namespaced.

}

return sprintf(
'<a href="%s" class="%s">%s</a>',
$url,
implode( ' ', $classes ),
$message
);
Copy link
Member

Choose a reason for hiding this comment

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

Let's do an apply_filters on the resulting HTML here.

}

}
14 changes: 14 additions & 0 deletions src/Uplink/Admin/Provider.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public function register_hooks() {
add_action( 'admin_enqueue_scripts', [ $this, 'store_admin_notices' ], 10, 1 );
add_action( 'admin_notices', [ $this, 'admin_notices' ], 10, 0 );
add_action( 'load-plugins.php', [ $this, 'remove_default_update_message' ], 50, 0 );
add_action( 'parse_request', [ $this, 'auth_request' ], 10, 1 );
}

/**
Expand All @@ -62,6 +63,18 @@ public function filter_plugins_api( $result, $action, $args ) {
return $this->container->get( Plugins_Page::class )->inject_info( $result, $action, $args );
}

/**
* Handle auth and disconnect requests to origin
*
* @param \WP $wp
*/
public function auth_request( $wp ) {
if ( ! is_user_logged_in() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of only logged in users, it might be a good idea to also check user capabilities current_user_can( 'manage_options' )

return;
}
$this->container->get( Actions::class )->handle_auth_request( $wp );
}

/**
* Filter the plugins transient.
*
Expand Down Expand Up @@ -94,6 +107,7 @@ public function ajax_validate_license() {
* @return void
*/
public function admin_init() {
$this->container->get( Actions::class )->register_route();
$this->container->get( License_Field::class )->register_settings();
}

Expand Down
16 changes: 16 additions & 0 deletions src/Uplink/Resources/Resource.php
Original file line number Diff line number Diff line change
Expand Up @@ -499,4 +499,20 @@ public function validate_license( $key = null, $do_network_validate = false ) {

return $results;
}

public function has_valid_auth_token( array $origin ) {
$token = get_option( sprintf( 'stellarwp_origin_%s_auth_token', $origin['slug'] ?? '' ), '' );
Copy link
Member

Choose a reason for hiding this comment

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

The option needs to be implementation-namespaced.


if ( empty( $token ) ) {
return false;
}

$token = json_decode( $token, true );

if ( empty( $token ) ) {
return false;
}

return $token['expiration'] > time();
}
}
6 changes: 6 additions & 0 deletions src/Uplink/Site/Data.php
Original file line number Diff line number Diff line change
Expand Up @@ -518,4 +518,10 @@ public function is_public(): bool {

return (bool) $is_public;
}

public function save_auth_token( string $data ) {
$data = json_decode( base64_decode( $data ), true );
update_option( sprintf( 'stellarwp_origin_%s_auth_token', $data['origin'] ?? '' ), json_encode( $data ) );
Copy link
Member

Choose a reason for hiding this comment

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

The option needs to be implementation-namespaced.

}

}
3 changes: 3 additions & 0 deletions src/admin-views/fields/settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* @var bool $show_title Whether to show the title. Default true.
*/

use StellarWP\Uplink\Admin\Auth;
use StellarWP\Uplink\Admin\License_Field;
use StellarWP\Uplink\Config;

Expand All @@ -14,6 +15,7 @@
}

$field = Config::get_container()->get( License_Field::class );
$auth = Config::get_container()->get( Auth::class );
$group = $field->get_group_name( sanitize_title( $plugin->get_slug() ) );

?>
Expand All @@ -23,6 +25,7 @@

<div class="stellarwp-uplink" data-js="stellarwp-uplink">
<div class="stellarwp-uplink__settings">
<?php echo $auth->do_auth_html(); ?>
Copy link
Member

Choose a reason for hiding this comment

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

How will this work when there's a single auth site for multiple plugins/add-ons?

Example use case:

This library will be included as part of The Events Calendar plugin. TEC has a number of addons that'll register themselves (using TEC's implmentation of the library) and may require authentication. We want a single login that applies to all plugins (add-ons) within that suite.

It looks like that is how this is implemented, but seeing the do_auth_html() call makes me think this might not be as generic as it needs to be for the above use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if we call this as static method in required place? Auth::do_auth_html()

<?php do_action( 'stellarwp/uplink/' . Config::get_hook_prefix(). '/license_field_before_form', $plugin->get_slug() ) ?>
<form method="post" action="options.php">
<?php settings_fields( $group ); ?>
Expand Down
49 changes: 49 additions & 0 deletions tests/wpunit/Resources/ResourceTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php declare( strict_types=1 );

namespace StellarWP\Uplink\Tests\Resources;

use StellarWP\Uplink\API\Validation_Response;
use StellarWP\Uplink\Register;
use StellarWP\Uplink\Uplink;

class ResourceTest extends \StellarWP\Uplink\Tests\UplinkTestCase {

public $resource;

public function setUp() {
parent::setUp();

$root = dirname( __DIR__, 3 );
$this->resource = Register::plugin(
'sample',
'Lib Sample',
$root . '/plugin.php',
Uplink::class,
'1.0.10',
Uplink::class
);
}

public function it_should_check_auth_token_valid() {
$result = $this->resource->has_valid_auth_token( [ 'slug' => 'sample' ] );
$this->assertFalse( $result );

update_option( sprintf( 'stellarwp_origin_%s_auth_token', 'sample' ), [
'token' => '11111',
'expiration' => strtotime( '-1 day'),
'origin' => '',
] );
$result = $this->resource->has_valid_auth_token( [ 'slug' => 'sample' ] );
$this->assertFalse( $result );

update_option( sprintf( 'stellarwp_origin_%s_auth_token', 'sample' ), [
'token' => '11111',
'expiration' => strtotime( '+1 day'),
'origin' => '',
] );

$result = $this->resource->has_valid_auth_token( [ 'slug' => 'sample' ] );
$this->assertTrue( $result );
}

}
14 changes: 14 additions & 0 deletions tests/wpunit/Site/DataTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,18 @@ public function it_should_collect_full_stats() {

remove_filter( 'stellarwp/uplink/test/use_full_stats', '__return_true' );
}

public function it_should_save_auth_token() {
$token = json_encode( [
'token' => '11111',
'expiration' => strtotime( '+1 day'),
'origin' => 'sample',
] );

$data = $this->container->make( Uplink\Site\Data::class );
$data->save_auth_token( base64_encode( $token ) );

$options_value = get_option( sprintf( 'stellarwp_origin_%s_auth_token', 'sample' ) );
$this->assertSame( $token, $options_value );
}
}