LICENS-45 Add auth logic #16

MlKilderkin · 2023-06-14T07:55:50Z

What

Add auth logic
Provide ability connect and disconnect from origin site
Do not allow download link if auth required.

https://github.com/stellarwp/licensing/pull/32
https://github.com/stellarwp/uplink-origin/pull/35

https://moderntribe.atlassian.net/browse/LICENS-45

tarecord

Really nice work, just a couple of suggestions below.

I also noticed that methods and properties don't have docblocks or the docblocks aren't filled out completely. Can you go back through and document those so these additions match the rest of the library?

/**
* Method Description.
*
* @since TBD
*
* @param 
*
* @return
*/

tarecord · 2023-06-27T18:49:43Z

src/Uplink/API/Validation_Response.php

+		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;


Some alignment inconsistencies here

tarecord · 2023-06-27T19:02:39Z

src/Uplink/API/Validation_Response.php

+			$url 		  = sprintf( '%s/stellarwp/oauth_connect/login?%s', $url, http_build_query( $query_params ) );
+
+			$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%' ),


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%' ),

tarecord · 2023-06-27T20:11:43Z

src/Uplink/Admin/Actions.php

+
+		$args = explode( '/', $wp->query_vars[ self::QUERY_VAR ] );
+
+		if (  ! empty( $args['disconnect'] ) ) { // @phpstan-ignore-line


What's the error you are ignoring here?

This is for Offset 'disconnect' on non-empty-array<int, string> in isset() does not exist.

tarecord · 2023-06-27T20:12:34Z

src/Uplink/Admin/Actions.php

+	}
+
+	/**
+	 * Remove auth token§


Suggested change

* Remove auth token§

* Remove auth tokens

tarecord · 2023-06-27T20:20:12Z

src/Uplink/Admin/Actions.php

+				'callback_uri' => urlencode( sprintf( '%s/stellarwp/connect', get_site_url() ) ),
+				'refer'		   => urlencode( wp_get_referer() ),


Alignment here

tarecord · 2023-06-27T20:26:31Z

src/Uplink/Admin/Auth.php

+
+		$token   = get_option( sprintf( 'stellarwp_origin_%s_auth_token', $license->origin->slug ?? '' ), '' );
+		$message = esc_html__( 'Connect to origin', '%TEXTDOMAIN%' );
+		$classes = [ 'button', 'button-primary' ];


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?

tarecord · 2023-06-27T20:43:26Z

src/Uplink/Admin/Provider.php

+	 * @param \WP $wp
+	 */
+	public function auth_request( $wp ) {
+		if ( ! is_user_logged_in() ) {


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

borkweb · 2023-06-28T14:51:52Z

src/Uplink/API/Validation_Response.php

+				'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 ) );


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.

borkweb · 2023-06-28T14:52:20Z

src/Uplink/Admin/Actions.php

+	public function handle_disconnect() {
+		$license = $this->get_license_object();
+
+		delete_option( sprintf( 'stellarwp_origin_%s_auth_token', $license->origin->slug ?? '' ) );


The option needs to be implementation-namespaced.

borkweb · 2023-06-28T14:53:27Z

src/Uplink/Admin/Actions.php

+			}
+
+			$query_params = [
+				'callback_uri' => urlencode( sprintf( '%s/stellarwp/connect', get_site_url() ) ),


This URL needs to be implementation-namespaced.

borkweb · 2023-06-28T14:53:37Z

src/Uplink/Admin/Auth.php

+		$plugin     = $collection->current();
+		$license    = $plugin->get_license_object();
+
+		$token   = get_option( sprintf( 'stellarwp_origin_%s_auth_token', $license->origin->slug ?? '' ), '' );


The option needs to be implementation-namespaced.

borkweb · 2023-06-28T14:54:30Z

src/Uplink/Admin/Auth.php

+		$license    = $plugin->get_license_object();
+
+		$token   = get_option( sprintf( 'stellarwp_origin_%s_auth_token', $license->origin->slug ?? '' ), '' );
+		$message = esc_html__( 'Connect to origin', '%TEXTDOMAIN%' );


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?

tests/wpunit/Site/DataTest.php

borkweb · 2023-06-28T15:00:25Z

src/Uplink/Admin/Actions.php

+
+		delete_option( sprintf( 'stellarwp_origin_%s_auth_token', $license->origin->slug ?? '' ) );
+
+		wp_safe_redirect( wp_get_referer() );


We should fire an action before the redirect.

borkweb · 2023-06-28T15:00:33Z

src/Uplink/Admin/Actions.php

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


We should fire an action before the redirect.

borkweb · 2023-06-28T15:01:16Z

src/Uplink/Admin/Actions.php

+
+		$args = explode( '/', $wp->query_vars[ self::QUERY_VAR ] );
+
+		if (  ! empty( $args['disconnect'] ) ) { // @phpstan-ignore-line


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

borkweb · 2023-06-28T15:02:04Z

src/Uplink/Admin/Auth.php

+			$url,
+			implode( ' ', $classes ),
+			$message
+		);


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

tarecord

Couple of notes, especially around hook naming.

tarecord · 2023-07-17T14:56:37Z

src/Uplink/Admin/Actions.php


 class Actions {

 	const QUERY_VAR = 'stellarwp_action';

 	/**
+	 * Register handle route for connect/disconnect
+	 *
+	 * @since 1.0.1


@borkweb how do we want to handle @since comments if the version hasn't been determined yet? We've used this in other libraries:

/** * Some really awesome method. * * @since TBD * * @return void */

tarecord · 2023-07-17T15:02:00Z

src/Uplink/API/Validation_Response.php

 			$query_params = [
-				'callback_uri' => urlencode( sprintf( '%s/stellarwp/connect', get_site_url() ) ),
+				'callback_uri' => urlencode( sprintf( '%s/%s', get_site_url(), Namespaces::get_hook_name( 'connect', '%TEXTDOMAIN%' ) ) ),


This is hard to understand what's going on. I'm not sure I understand why the Namespaces class is necessary?

tarecord · 2023-07-17T15:09:48Z

src/Uplink/Utils/Namespaces.php

I don't think this is necessary because it's adding a lot of additional abstraction to hooks (you're having to figure out what the hook is doing by referencing this class).

We've dealt with this a bit in the Telemetry library too where plugins can hook into their own implementation without affecting others.

Instead of adding a prefix to the hook name, we've adjusted to pass the plugin slug as an argument to the hook:

apply_filters( 'your/hook/name', '[plugin-slug]', 'value' ); do_action( 'your/hook/name', '[plugin-slug]' );

Since the plugin slug is passed in the hooks, plugins can validate that the hook is firing for a specific implementation:

add_filter( 'the/hook/name', 'filter_something', 10, 2 ); function filter_something( $plugin_slug, $value ) { if ( 'the-events-calendar' !== $plugin_slug) { return $value; } // Do value adjustment here... return $value; }

Once all the hooks are updated to pass in the plugin slug, hook names shouldn't have to change and they'll be simpler to understand in the library 😄

@tarecord maybe I understood the idea for #16 (comment) in wrong way #16 (comment)

You didn't understand wrong, a namespace is perfect when using them for options and endpoints.

I am only referring to the filters/actions you are adding a namespace to.

I did notice that you have Config::get_hook_prefix() available to you. Would that be good to use instead of this class? I imagine brands will want to define their prefix once and have it used everywhere

Co-authored-by: Matthew Batchelder <[email protected]>

LICENS-45 Add auth logic

25c7909

MlKilderkin self-assigned this Jun 14, 2023

MlKilderkin added the Attention required label Jun 14, 2023

MlKilderkin added 8 commits June 14, 2023 15:57

LICENS-45 Add link to upgrade notices

f97b940

LICENS-45 Add link to upgrade notices

c9da919

LICENS-45 Add link to upgrade notices

d457408

LICENS-45 Decode response

2bf67dc

LICENS-45 Add url encode for params

71c61bc

LICENS-45 Fix issues with PHPStan

772ddfb

LICENS-45 Add tests

232bc45

LICENS-45 ignore phpstan

6eaa2f7

tarecord requested changes Jun 27, 2023

View reviewed changes

borkweb requested changes Jun 28, 2023

View reviewed changes

MlKilderkin added 5 commits July 17, 2023 12:18

LICENS-45 Resolve CR comments

e0c3a89

LICENS-45 Resolve CR comments

3ab0da2

LICENS-45 Fix tests

9427972

LICENS-45 Resovle comments

6091a4e

LICENS-45 Fix tests

90b2bff

MlKilderkin requested review from borkweb and tarecord July 17, 2023 13:21

tarecord requested changes Jul 17, 2023

View reviewed changes

MlKilderkin requested a review from tarecord July 21, 2023 16:01

MlKilderkin and others added 3 commits July 31, 2023 16:14

LICENS-45 Resolve CR comments

0693a93

Update tests/wpunit/Resources/ResourceTest.php

1f293c6

Co-authored-by: Matthew Batchelder <[email protected]>

Update tests/wpunit/Site/DataTest.php

4298871

Co-authored-by: Matthew Batchelder <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LICENS-45 Add auth logic #16

LICENS-45 Add auth logic #16

MlKilderkin commented Jun 14, 2023 •

edited

Loading

tarecord left a comment

tarecord Jun 27, 2023

tarecord Jun 27, 2023

tarecord Jun 27, 2023

MlKilderkin Jul 17, 2023

tarecord Jun 27, 2023

tarecord Jun 27, 2023

tarecord Jun 27, 2023

tarecord Jun 27, 2023

borkweb Jun 28, 2023

borkweb Jun 28, 2023

borkweb Jun 28, 2023

borkweb Jun 28, 2023

borkweb Jun 28, 2023

borkweb Jun 28, 2023

borkweb Jun 28, 2023

borkweb Jun 28, 2023

borkweb Jun 28, 2023

tarecord left a comment

tarecord Jul 17, 2023

tarecord Jul 17, 2023

tarecord Jul 17, 2023 •

edited

Loading

MlKilderkin Jul 18, 2023

tarecord Jul 24, 2023

MlKilderkin Jul 31, 2023

	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%' ),


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

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

		'callback_uri' => urlencode( sprintf( '%s/stellarwp/connect', get_site_url() ) ),
		'refer' => urlencode( wp_get_referer() ),


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

		wp_safe_redirect( wp_get_referer() );

LICENS-45 Add auth logic #16

Are you sure you want to change the base?

LICENS-45 Add auth logic #16

Conversation

MlKilderkin commented Jun 14, 2023 • edited Loading

What

tarecord 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

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

tarecord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarecord Jul 17, 2023 • edited Loading

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

MlKilderkin commented Jun 14, 2023 •

edited

Loading

tarecord Jul 17, 2023 •

edited

Loading