Support Multiple tokens #73

dpanta94 · 2024-07-16T12:43:57Z

Modify library in order to be able to support multiple oAuth Tokens.

Current implementation would assume always ONE oAuth field per instance of the Uplink Library, which for the case of TEC for example can be problematic since every next auth would overwrite every previous.

Support Multiple Tokens - Tests

tarecord · 2024-07-16T20:01:38Z

tests/wpunit/Auth/Token/MultipleTokensTest.php

+	/**
+	 * @test
+	 */
+	public function it_should_have_backwards_compatibility_with_single_and_multiple_tokens(): void {


Does this backwards compatibility test check that this works with existing site tokens?
It seems like it's only checking that the token option can be updated with a single token vs multiple tokens.

We want to make sure that the Token_Manager is able to retrieve the old tokens right?

i.e.

update_network_option( get_current_network_id(), 'custom_uplink_auth_token', 'single-token'); // Token_Manager::get() without slug. $this->assertSame( 'single-token', $this->token_manager->get() );

Good call, I'll either add in another test or alter the above to add in this logic.

This is being covered by existing tests though. see for example test_it_stores_and_retrieves_a_token_from_the_network in tests/wpunit/Auth/Token/CustomDomainMultisiteTokenMangerTest.php and test_it_stores_and_retrieves_a_token in tests/wpunit/Auth/Token/SingleSiteTokenMangerTest.php

I added one more set of tests in this PR #75

…ckwards-compat-tests Additional Backwards Compatability Test

defunctl

Thanks for doing this!

I have an older PR Kadence is using right now where this might affect some things: #51

I wonder if this should be adjusted to pass around a Resource instance instead, that way we get access to everything on that resource without having to add additional parameters to methods.

e.g.

Of course, this means the Token Manager should be taking in a nullable Resource instance where we can then use $resource->slug if not null and still provided that backwards compatibility.

Let me know if you have any questions or want to discuss.

defunctl · 2024-07-17T14:21:12Z

tests/wpunit/Auth/Token/MultipleTokensTest.php

+	 * @test
+	 */
+	public function it_should_have_backwards_compatibility_with_single_and_multiple_tokens(): void {
+		$collection = Config::get_container()->get( Collection::class );


I don't think this is used anywhere in this test.

defunctl · 2024-07-17T14:22:54Z

src/Uplink/Auth/Token/Token_Manager.php

+	 *
+	 * @return null|string|array
+	 */
+	public function get_all() {


Trying to think about what to do about this. On one hand, we've already modified the existing Token_Manager Contract, but in a backwards compatible way.

So, rather than just adding this to the contract, I think we're going to need a new Multiple_Token_Manager interface that extends the existing Token_Manager Contract and the container bindings updated and pass around that new interface instead so anyone asking for the original contract externally shouldn't be affected.

Hey @defunctl ! First of all thank you so much for your detailed review.

Have a question about this. The reason i didn't add this in the existing interface (or any other interface) in the first place, is that this method is acting in a way that serves both installations storing a single token or multiple tokens.

So it could return

null when no option stored
string when still in the schema of storing a single token only
array when storing multiple tokens in the same option.

In the interface i cant declare mixed as the return type since 7.X version of php don't support it.

I could still create an interface but without type hinting the return value. Let me know what do you think please.

Also i don't think we should update the binding in the container. Rather i would update Token_Manager to implement the new interface instead.

Since Token_Manager is a final class i am thinking there shouldn't be any dangers doing that.

defunctl · 2024-07-17T14:36:01Z

src/Uplink/Auth/Token/Token_Manager.php

+	 * @return string
+	 */
+	protected function validate_slug( string $slug = '' ): string {
+		return $slug && Config::get_container()->get( Collection::class )->offsetExists( $slug ) ? $slug : '';


We should utilize dependency injection instead of a service locator anti-pattern, pass Collection to the constructor of this class instead.

Update the container bindings to fetch the singleton instance.

Suggested change

return $slug && Config::get_container()->get( Collection::class )->offsetExists( $slug ) ? $slug : '';

return $slug && $this->collection->offsetExists( $slug ) ? $slug : '';

defunctl · 2024-07-17T14:38:05Z

tests/wpunit/Auth/Token/MultipleTokensTest.php

+	/**
+	 * @before
+	 */
+	protected function multiple_tokens_setup(): void {


Given my DI suggestion for the Token Manager, this entire method should be replaced with the following so we are using the correct container instance that all tests are using:

/** * @var Collection */ private $collection; protected function setUp(): void { parent::setUp(); Config::set_token_auth_prefix( 'custom_' ); // Run init again to reload the Token/Provider. Uplink::init(); $this->collection = $this->container->get( Collection::class ); $this->token_manager = $this->container->get( Token_Manager::class ); }

The protected function multiple_tokens_setup(): void method can be completely removed now.

defunctl · 2024-07-17T14:40:40Z

tests/wpunit/Auth/Token/MultipleTokensTest.php

+	 * @return mixed
+	 */
+	public function setup_container_get_slug( array $resource ) {
+		$collection = Config::get_container()->get( Collection::class );


Suggested change

$collection = Config::get_container()->get( Collection::class );

defunctl · 2024-07-17T14:48:30Z

tests/wpunit/Auth/Token/SubDomainMultisiteTokenMangerTest.php

@@ -48,7 +48,7 @@ public function test_it_stores_and_retrieves_a_token_from_the_network(): void {

 		$this->assertSame( $token, $this->token_manager->get() );

-		$this->assertSame( $token, get_network_option( get_current_network_id(), $this->token_manager->option_name() ) );
+		$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $this->token_manager->option_name() ) )['0'] );


Suggested change

$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $this->token_manager->option_name() ) )['0'] );

$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $this->token_manager->option_name() ) )[0] );

defunctl · 2024-07-17T14:48:55Z

tests/wpunit/Auth/Token/SubfolderMultisiteTokenMangerTest.php

@@ -48,7 +48,7 @@ public function test_it_stores_and_retrieves_a_token_from_the_network(): void {

 		$this->assertSame( $token, $this->token_manager->get() );

-		$this->assertSame( $token, get_network_option( get_current_network_id(), $this->token_manager->option_name() ) );
+		$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $this->token_manager->option_name() ) )['0'] );


Suggested change

$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $this->token_manager->option_name() ) )['0'] );

$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $this->token_manager->option_name() ) )[0] );

defunctl · 2024-07-17T14:51:39Z

tests/wpunit/Auth/Token/CustomDomainMultisiteTokenMangerTest.php

@@ -48,7 +48,7 @@ public function test_it_stores_and_retrieves_a_token_from_the_network(): void {

 		$this->assertSame( $token, $this->token_manager->get() );

-		$this->assertSame( $token, get_network_option( get_current_network_id(), $this->token_manager->option_name() ) );
+		$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $this->token_manager->option_name() ) )['0'] );


Suggested change

$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $this->token_manager->option_name() ) )['0'] );

$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $this->token_manager->option_name() ) )[0] );

defunctl · 2024-07-17T14:51:52Z

tests/wpunit/Auth/AuthorizerTest.php

@@ -73,7 +73,7 @@ public function test_it_does_not_authorize_a_subsite_with_an_existing_network_to
 		// Store a token while on the main site.
 		$this->assertTrue( $token_manager->store( '695be4b3-ad6e-4863-9287-3052f597b1f6' ) );

-		$this->assertSame( $token, get_network_option( get_current_network_id(), $token_manager->option_name() ) );
+		$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $token_manager->option_name() ) )['0'] );


Suggested change

$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $token_manager->option_name() ) )['0'] );

$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $token_manager->option_name() ) )[0] );

Missed this one.

defunctl · 2024-07-17T15:02:38Z

src/Uplink/Auth/Token/Token_Manager.php

+		$slug = $this->validate_slug( $slug );
+
+		if ( ! $slug ) {
+			return delete_network_option( get_current_network_id(), $this->option_name );


Is this saying that if we don't have the slug in the resource collection, we delete all the data?

Wouldn't that delete other slug data as well or am I missing something?

Fix/multiple tokens issues

…iple-tokens/storage-strategy

…ssage

…rategy Storage System to replace Transients

src/Uplink/Auth/Token/Token_Manager.php

defunctl · 2024-07-17T20:46:17Z

tests/wpunit/Auth/AuthorizerTest.php

@@ -73,7 +73,7 @@ public function test_it_does_not_authorize_a_subsite_with_an_existing_network_to
 		// Store a token while on the main site.
 		$this->assertTrue( $token_manager->store( '695be4b3-ad6e-4863-9287-3052f597b1f6' ) );

-		$this->assertSame( $token, get_network_option( get_current_network_id(), $token_manager->option_name() ) );
+		$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $token_manager->option_name() ) )['0'] );


Missed this one.

defunctl · 2024-07-17T20:47:06Z

tests/wpunit/Auth/Token/MultipleTokensTest.php

+	/**
+	 * @before
+	 */
+	protected function multiple_tokens_setup(): void {


The protected function multiple_tokens_setup(): void method can be completely removed now.

Co-authored-by: Justin <[email protected]>

Amend CR comments for field/form APIs

Support Multiple tokens

887e73a

dpanta94 added the enhancement New feature or request label Jul 16, 2024

dpanta94 requested a review from redscar July 16, 2024 12:43

dpanta94 self-assigned this Jul 16, 2024

dpanta94 added 6 commits July 16, 2024 15:47

Validate that slug has a value before checking if there is a resource

38e4223

Updated Inteface's delete method

f34ebbf

Fix type hint issues with php 7.4

ba7d0a4

Check for null prior to array

ea75b63

Fixing Tests

f4729e5

Fixing phpstan

6da15d6

tarecord requested review from borkweb and tarecord July 16, 2024 13:33

dpanta94 and others added 3 commits July 16, 2024 16:36

Fixing phpstan

a5fb1fe

Fixing more tests

045c129

First set of working tests.

a93186d

redscar mentioned this pull request Jul 16, 2024

Support Multiple Tokens - Tests #74

Merged

redscar and others added 3 commits July 16, 2024 15:10

Method names and switching to @before

a5e8c38

Removed the parent:setup().

3a8e6e3

Merge pull request #74 from stellarwp/feat/support-multiple-tokens-tests

e9f8704

Support Multiple Tokens - Tests

dpanta94 marked this pull request as ready for review July 16, 2024 19:28

tarecord reviewed Jul 16, 2024

View reviewed changes

borkweb requested a review from defunctl July 17, 2024 03:04

Added an additional test for backwards compatability.

0edb446

redscar mentioned this pull request Jul 17, 2024

Additional Backwards Compatability Test #75

Merged

redscar and others added 2 commits July 17, 2024 08:58

Added support for multisites.

8d52c8a

Merge pull request #75 from stellarwp/feat/support-multiple-tokens-ba…

ac3d458

…ckwards-compat-tests Additional Backwards Compatability Test

defunctl requested changes Jul 17, 2024

View reviewed changes

redscar mentioned this pull request Jul 17, 2024

Multiple Tokens - Tests #76

Merged

dpanta94 added 2 commits July 17, 2024 22:06

Code review changes

84fa56c

Removing debug

3b67cf6

dpanta94 and others added 19 commits July 18, 2024 22:56

Bumb prio of executing store/delete

3d38782

Change Action Prio

2248466

Merge pull request #78 from stellarwp/fix/multiple-tokens-issues

6e8e276

Fix/multiple tokens issues

Implement storage system and remove all direct transient usage

cdbb66a

Merge branch 'refs/heads/feat/support-multiple-tokens' into feat/mult…

b9ed6cc

…iple-tokens/storage-strategy

Fix test not running in singlesite mode

5dca88b

Allow the storage driver to be configured.

cd04a0d

remove duplicate braces

38668f9

Fix phpstan

9f2ee7d

Remove unneeded setUp method

89b7c3c

Set the default storage option out of the box

e49088f

Check the configured storage driver before showing litespeed error me…

443d933

…ssage

Merge pull request #79 from stellarwp/feat/multiple-tokens/storage-st…

58bab89

…rategy Storage System to replace Transients

Amend CR comments for field/form APIs

28eb2a5

Migrated field/form from admin-views to views

1fd1680

Updates Field's API

24b96b8

Add method to grab Field instance from resource

57183c0

Fix tests

ca61f9d

Remove unnecessary blank line

a52184a

defunctl approved these changes Jul 19, 2024

View reviewed changes

dpanta94 and others added 8 commits July 21, 2024 19:30

Update src/Uplink/Admin/Fields/Field.php

3906c2b

Co-authored-by: Justin <[email protected]>

Update src/Uplink/Admin/Fields/Field.php

2555a9b

Co-authored-by: Justin <[email protected]>

Update src/Uplink/Admin/Fields/Field.php

cd5d2c3

Co-authored-by: Justin <[email protected]>

Refactor from get_group_name to get_name

f64f551

Refactor function get_field

47207b0

Fix tests

5b034ea

Refactor from Assets to Asset_Manager

9a74d20

Merge pull request #80 from stellarwp/fix/field-form-api-review

a739018

Amend CR comments for field/form APIs

dpanta94 merged commit 4375b18 into feat/fluent-form-builder Jul 22, 2024
13 checks passed

dpanta94 deleted the feat/support-multiple-tokens branch July 22, 2024 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Multiple tokens #73

Support Multiple tokens #73

dpanta94 commented Jul 16, 2024

tarecord Jul 16, 2024

redscar Jul 16, 2024

dpanta94 Jul 17, 2024 •

edited

Loading

redscar Jul 17, 2024

defunctl left a comment

defunctl Jul 17, 2024

defunctl Jul 17, 2024

dpanta94 Jul 17, 2024

dpanta94 Jul 17, 2024

defunctl Jul 17, 2024

defunctl Jul 17, 2024 •

edited

Loading

defunctl Jul 17, 2024

defunctl Jul 17, 2024

defunctl Jul 17, 2024

defunctl Jul 17, 2024

defunctl Jul 17, 2024

defunctl Jul 17, 2024

defunctl Jul 17, 2024

defunctl Jul 17, 2024

defunctl Jul 17, 2024

defunctl Jul 17, 2024

	return $slug && Config::get_container()->get( Collection::class )->offsetExists( $slug ) ? $slug : '';
	return $slug && $this->collection->offsetExists( $slug ) ? $slug : '';

	$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $this->token_manager->option_name() ) )['0'] );
	$this->assertSame( $token, array_values( get_network_option( get_current_network_id(), $this->token_manager->option_name() ) )[0] );

Support Multiple tokens #73

Support Multiple tokens #73

Conversation

dpanta94 commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpanta94 Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defunctl 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

defunctl Jul 17, 2024 • 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

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

dpanta94 Jul 17, 2024 •

edited

Loading

defunctl Jul 17, 2024 •

edited

Loading