[API Pull] Auth Flow #2424

puntope · 2024-06-07T09:44:17Z

Changes proposed in this Pull Request:

Auth flow for granting access to WPCOM after enabling the product sync feature
Tweak the banner
Revoke access when disabling the feature

Detailed test instructions:

Go to Settings and verify you see "We will soon transition to a new and improved method for synchronizing product data with Google -- Get early access" banner
Click on Get early access (see the button loading)
You will be redirected to auth flow
Complete the auth flow
You will be redirected back to the store
You see "Google has been granted access to fetch your product data." in green on the Google MC card
Click on "Disable product data fetch"
Auth is disabled and you see the banner again

Additional details:

Deeper unit Tests coming on further PRs
Consistency and more tracking events coming in further PRs
Notice that the errors are still not handled from Google. You can see that for example removing the nonce param in get_data_from_google function

jorgemd24

Thanks @puntope for working on this. I tested it along with https://github.com/Automattic/woocommerce-connect-server/pull/2485 , and it worked well except that after authorizing the WPCOM App, it seems Google is redirecting to the wrong URL.

However, the OAuth flow was successful, as I could see the GLA App connected to my site. I then went to the settings to disconnect the API pull, and it successfully revoked the token.

I left some minor comments about the code too.

src/API/Google/Middleware.php

jorgemd24 · 2024-06-11T17:09:14Z

src/API/WP/OAuthService.php

+
+		$response = Client::remote_request( $args );
+
+		if ( is_wp_error( $response ) ) {


Shouldn't we check if the HTTP status code is >= 200? For example, if there are no tokens, the server will respond with status 500 - Internal Server Error. - {"code":"wpcom_partner_token_not_associated","message":"No token found associated with the client ID and user.","data":null}

Fixed here. Thx ece73b1

jorgemd24 · 2024-06-11T17:20:43Z

tests/Unit/API/WP/OAuthServiceTest.php

@@ -3,9 +3,14 @@

 namespace Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\API\WP;

+use Automattic\WooCommerce\GoogleListingsAndAds\API\Google\Ads;


Unused dependency

Fixed here thx. 436881c

ianlin · 2024-06-14T07:06:47Z

after authorizing the WPCOM App, it seems Google is redirecting to the wrong URL.

Had a look at it and I found they tried to redirect us to this URL:

https://shoppingdataintegration.googleapis.com/v1/credentials/partners/WooCommerce/https%3A%2F%2Ftest-site.example.com%2Fwp-admin%2Fadmin.php%3Fpage%3Dwc-admin%26path%3D%2Fgoogle%2Fsettings%3Fnonce=XXXXX&google_wpcom_app_status=approved

We should ask them to remove https://shoppingdataintegration.googleapis.com/v1/credentials/partners/WooCommerce/ so they could redirect to the correct URL.

However I also noticed there were something wrong in the merchant site's URL:

The URL decoded version of the merchant site's URL is:

https://test-site.example.com/wp-admin/admin.php?page=wc-admin&path=/google/settings?nonce=XXXXX&google_wpcom_app_status=approved

where it added a question mark ? before nonce parameter which it would cause the page not found. We should also ask them the replace that ? with & so it would return to the merchant site correctly.

puntope · 2024-06-14T14:06:11Z

Thanks, @ianlin and @jorgemd24 for your suggestions and investigation. They fixed the error from Google and now seems to work. Also, I addressed the review comments. Can you test again?

jorgemd24 · 2024-06-18T20:11:47Z

@puntope, I tested it, and it is now redirecting me back to my site. Is there anything else we need to wait for, or can I proceed with the review? Also, I noticed some PHPCS errors.

puntope · 2024-06-19T08:32:51Z

@puntope, I tested it, and it is now redirecting me back to my site. Is there anything else we need to wait for, or can I proceed with the review? Also, I noticed some PHPCS errors.

We need to wait for a fix from google. I will ping you

codecov · 2024-06-19T08:37:21Z

Codecov Report

Attention: Patch coverage is 64.70588% with 18 lines in your changes missing coverage. Please review.

Project coverage is 64.3%. Comparing base (aec6d57) to head (e42f728).
Report is 481 commits behind head on feature/google-api-project.

Additional details and impacted files

@@                      Coverage Diff                       @@
##             feature/google-api-project   #2424     +/-   ##
==============================================================
+ Coverage                          64.2%   64.3%   +0.1%     
- Complexity                         4484    4549     +65     
==============================================================
  Files                               471     794    +323     
  Lines                             18862   22795   +3933     
  Branches                              0    1219   +1219     
==============================================================
+ Hits                              12117   14665   +2548     
- Misses                             6745    7963   +1218     
- Partials                              0     167    +167

Flag	Coverage Δ
js-unit-tests	`63.5% <ø> (?)`
php-unit-tests	`64.6% <64.7%> (+0.3%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/API/Google/Middleware.php	`95.0% <100.0%> (+8.2%)`	⬆️
src/MerchantCenter/AccountService.php	`98.3% <100.0%> (+2.1%)`	⬆️
...rc/API/Site/Controllers/RestAPI/AuthController.php	`92.4% <0.0%> (-1.0%)`	⬇️
src/API/WP/OAuthService.php	`66.7% <33.3%> (-14.0%)`	⬇️

... and 512 files with indirect coverage changes

puntope · 2024-06-24T16:14:14Z

@jorgemd24 @ianlin This seems to be ready for another look. Google deployed their last changes

jorgemd24

Thanks @puntope for working on this. I tested the happy path and everything works well. However, if the response from WCS is anything other than 200, I get redirected to wp-admin/undefined.

jorgemd24 · 2024-06-25T18:37:38Z

src/API/WP/OAuthService.php

+		$request = Client::remote_request( $args );
+
+		if ( is_wp_error( $request ) ) {
+			throw new Exception( $request->get_error_message(), $request->get_error_code() );


The Exception expects the second parameter to be an int, but wp_error accepts the code as either a string or int so if wp_error had set the code as a string it will throw a fatal.

Tweaked here. I will return just 400

e42f728

jorgemd24 · 2024-06-25T18:43:13Z

src/API/WP/OAuthService.php

+	}
+
+	/**
+	 * Calls an API by Google via WCS to get required information in order to form an auth URL.
 	 *
 	 * @return array{client_id: string, redirect_uri: string, nonce: string} An associative array contains required information that is retrived from Google.
 	 * client_id:    Google's WPCOM app client ID, will be used to form the authorization URL.
 	 * redirect_uri: A Google's URL that will be redirected to when the merchant approve the app access. Note that it needs to be matched with the Google WPCOM app client settings.
 	 * nonce:        A string returned by Google that we will put it in the auth URL and the redirect_uri. Google will use it to verify the call.
 	 */
 	protected function get_data_from_google(): array {


get_sdi_auth_params can return an exception.

tweaked here

e42f728

jorgemd24 · 2024-06-25T18:49:56Z

src/API/Google/Middleware.php

+			$result   = $client->get( $this->get_sdi_auth_endpoint() );
+			$response = json_decode( $result->getBody()->getContents(), true );
+
+			if ( 200 !== $result->getStatusCode() ) {


I simulated a response other than 200, but I don't see any error and I get redirected to: /wp-admin/undefined

Hi @jorgemd24 How did you simulate the bad response?

When I do so I cannot reproduce the error you mentioned.

Screen.Recording.2024-06-26.at.14.01.27.mov

I did the simulation doing:

$result = new Response(400 ); $response = [];

Ah sorry! I realized that my simulation was not correct, because I changed this line from if ( 200 !== $result->getStatusCode() ) { to if ( 200 === $result->getStatusCode() ) { but then the Exception was thrown as 200 and that's why I was getting that result.

jorgemd24 · 2024-06-25T18:59:36Z

Also, what do you think about setting the default value of this filter to true? Or do we want the merchant to enable this manually?

google-listings-and-ads/src/API/WP/NotificationsService.php

Line 176 in ec1e862

return apply_filters( 'woocommerce_gla_notifications_enabled', false );

jorgemd24 · 2024-06-25T20:14:48Z

One more thought: how will Google return any errors through the OAuth flow, and how are we going to handle them?

puntope · 2024-06-26T07:37:05Z

Or do we want the merchant to enable this manually?

If nothing has changed for now I think the first version requires setting the filter. But not sure if that changed after dark launch discussion

puntope · 2024-06-26T07:39:28Z

One more thought: how will Google return any errors through the OAuth flow, and how are we going to handle them?

The discussion between @ianlin and Google was that they would return google_wpcom_app_status=error. Can you confirm @ianlin ?

puntope · 2024-06-26T10:13:12Z

Thanks @jorgemd24. Can you take another look ?

jorgemd24

Thanks for the adjustments, LGTM 👍

If nothing has changed for now I think the first version requires setting the filter. But not sure if that changed after dark launch discussion

Okay I will ask them, for me it looks a bit redundant.

The discussion between @ianlin and Google was that they would return google_wpcom_app_status=error. Can you confirm @ianlin ?

If so, I can work on a PR to display those errors to the user.

puntope · 2024-06-26T17:58:28Z

Thanks for the adjustments, LGTM 👍

If nothing has changed for now I think the first version requires setting the filter. But not sure if that changed after dark launch discussion

Okay I will ask them, for me it looks a bit redundant.

Sounds good. @jorgemd24 Go ahead and we can remove if so.

The discussion between @ianlin and Google was that they would return google_wpcom_app_status=error. Can you confirm @ianlin ?

If so, I can work on a PR to display those errors to the user.

@jorgemd24 We do display a notice on the card banner and allow granting again. Is this what you mean?

p.s You can also check the tracking topic if you are looking for issues.

ianlin · 2024-06-27T04:12:58Z

One more thought: how will Google return any errors through the OAuth flow, and how are we going to handle them?

The discussion between @ianlin and Google was that they would return google_wpcom_app_status=error. Can you confirm @ianlin ?

@puntope @jorgemd24 Yeah correct, they will just set the query param to google_wpcom_app_status=error without what error it is.

puntope added 8 commits May 28, 2024 17:03

Get Auth params with SDI

1b2ff1f

Merge branch 'feature/google-api-project' into add/auth-flow

8a769ba

Add WPCOM API OAuth revoke endpoint logic

74fd4cb

Add SDI call for getting thr Google OAuth params

3d8c043

Tweak Notifications feature banner

0abe152

Add tests for middleware sdi

74300c5

Tweak tests for OauthService

aa33687

PHPCS for tests

b4173b6

puntope changed the title ~~Auth Flow~~ [API Pull] Auth Flow Jun 7, 2024

github-actions bot added changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement. labels Jun 7, 2024

jorgemd24 assigned puntope Jun 11, 2024

jorgemd24 reviewed Jun 11, 2024

View reviewed changes

jorgemd24 mentioned this pull request Jun 13, 2024

API Pull - Revoke access if plugin is deactivated #2430

Merged

puntope added 5 commits June 14, 2024 17:30

Add loading in banner

3d5aa55

Fix docs

5b9a462

Remove unused dependency

436881c

Handle errors

ece73b1

PHPCS

ca4e95c

puntope requested a review from jorgemd24 June 14, 2024 14:06

puntope marked this pull request as ready for review June 14, 2024 14:06

puntope added 3 commits June 14, 2024 18:40

Tweak error handling

f70f89e

PHPCS

bba7b0c

Fix error when revoke fails

6fee5bd

puntope added 2 commits June 19, 2024 11:20

Update auth generate URL

bfdca07

Tweak tests

e6fd185

PHPCS

b9ce5da

puntope added 2 commits June 24, 2024 19:01

Merge branch 'feature/google-api-project' into add/auth-flow

d156117

PHPCS

08c9b61

Remove unused old tests

ec1e862

jorgemd24 reviewed Jun 25, 2024

View reviewed changes

Document exceptions

e42f728

jorgemd24 approved these changes Jun 26, 2024

View reviewed changes

puntope merged commit b085f64 into feature/google-api-project Jun 26, 2024
15 checks passed

puntope deleted the add/auth-flow branch June 26, 2024 17:45

jorgemd24 mentioned this pull request Jun 28, 2024

Google API Project #2146

Open

35 tasks


		$response = Client::remote_request( $args );

		if ( is_wp_error( $response ) ) {

		@@ -3,9 +3,14 @@

		namespace Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\API\WP;

		use Automattic\WooCommerce\GoogleListingsAndAds\API\Google\Ads;

[API Pull] Auth Flow #2424

[API Pull] Auth Flow #2424

Uh oh!

Conversation

puntope commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Detailed test instructions:

Additional details:

Uh oh!

jorgemd24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianlin commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puntope commented Jun 14, 2024

Uh oh!

jorgemd24 commented Jun 18, 2024

Uh oh!

puntope commented Jun 19, 2024

Uh oh!

codecov bot commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

puntope commented Jun 24, 2024

Uh oh!

jorgemd24 left a comment

Choose a reason for hiding this comment

Uh oh!

jorgemd24 Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgemd24 Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgemd24 commented Jun 25, 2024

Uh oh!

jorgemd24 commented Jun 25, 2024 • edited by puntope Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puntope commented Jun 26, 2024

Uh oh!

puntope commented Jun 26, 2024

Uh oh!

puntope commented Jun 26, 2024

Uh oh!

jorgemd24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

puntope commented Jun 26, 2024

Uh oh!

ianlin commented Jun 27, 2024

Uh oh!

Uh oh!

puntope commented Jun 7, 2024 •

edited

Loading

ianlin commented Jun 14, 2024 •

edited

Loading

codecov bot commented Jun 19, 2024 •

edited

Loading

jorgemd24 Jun 25, 2024 •

edited

Loading

jorgemd24 Jun 25, 2024 •

edited

Loading

jorgemd24 commented Jun 25, 2024 •

edited by puntope

Loading