[REST API] Alamofire network retry and adapt network requests #8483

selanthiraiyan · 2022-12-27T08:39:48Z

Closes: #8482

Description

As part of the REST API project we need the capability to retry a request in case of authentication failure.

The flow for a REST API request is described below,

Send a request
If an error occurs due to authentication failure for a REST request.
Regenerate a new application password.
Retry the REST API request again.

The request adapt and retry implementation is followed from https://github.com/wordpress-mobile/WordPressKit-iOS/blob/trunk/WordPressKit/Authenticator.swift

There are no changes to how a Jetpack or WPCOM request is handled. They will be authenticated using the new RequestAuthenticator. But there will be no retrying.

UI tests

As the network started validating the requests using Alamofire's validate method like this, the UI tests started failing for mocks without headers section.

The UI tests were failing due to incorrect Content-Type in the response. The headers section takes care of adding the Content-Type to the mock response.

To fix this, I added the headers part for all missing mock JSON files.

Testing instructions

Login into the app using WPCOM credentials. (Both Email and Store Address flows)
Do smoke testing to ensure that everything works as before.
We don't have any REST API requests migrated. So there is no testing required. Coupons endpoints will be migrated in a future PR.

Screenshots

NA

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…I using application password.

…RLRequest` instead of `URLRequestConvertible`.

…equests if applicable.

…entication.

…able for REST API.

This reverts commit 6e8efb9.

wpmobilebot · 2022-12-27T08:58:14Z

You can test the changes from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr8483-c66416d on your iPhone

If you need access to App Center, please ask a maintainer to add you.

jaclync

Hi @selanthiraiyan 👋 Sorry for some preliminary questions before diving into deeper code review, since I don't know much about this project. Trying to clarify a few things:

What does the user provide when signing in with the application password for the REST API feature?
From the issue description:

WPOrg REST API requests should be authenticated using application password. If there is an authentication failure, we should generate application password and retry the REST API request.

What can trigger the original application password to fail/change, is it a common scenario? Just trying to understand why we need to handle this API failure case only for this new authentication method.

What does the adapter do? I'm not very familiar with the technical requirements, just wanted to get a full understanding of the changes

If there's a p2 or reference that explains the answers to my questions above, feel free to just drop the link here 🙂

This reverts commit cf8a394.

This reverts commit 5e35c3e.

peril-woocommerce · 2022-12-28T11:08:03Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

…()`.

…equest properly.

selanthiraiyan · 2022-12-29T01:54:59Z

Thanks for the review, @jaclync!

I have addressed your comments. The PR is ready for the second pass of review. 🙏

jaclync · 2022-12-29T02:46:40Z

https://make.wordpress.org/core/2020/11/05/application-passwords-integration-guide/ has the details and screenshot about how the UI looks in wp-admin.

From the wp-admin UI here, it looks like it shows the name of the app with a CTA to revoke. If the user chooses to revoke the application password here, it feels like they intentionally want to remove access from the app? If that's the case, then it feels a bit intrusive how the app just creates another password. I'm sure this has been thought out before, just sharing my observations from the review.

jaclync

I think I have more understanding of the PR now 😄 thanks for the updates and explanations, I took another pass with some questions/suggestions.

Networking/Networking/ApplicationPassword/RequestAuthenticator.swift

Networking/Networking/Requests/RESTRequest.swift

jaclync · 2022-12-29T03:16:57Z

Networking/Networking/ApplicationPassword/RequestConverter.swift

+    let credentials: Credentials?
+
+    func convert(_ request: URLRequestConvertible) -> URLRequestConvertible {
+        guard let jetpackRequest = request as? JetpackRequest,


just wondering, do we only do the conversation from JetpackRequest? How about DotcomRequest and WordPressOrgRequest?

DotcomRequest are sent to WordPress.com servers, and it needs WPCOM authentication token. We cannot talk to WPCOM servers using the application password.

WordPressOrgRequest will be sent through WordPressOrgNetwork instead of AlamofireNetwork.

WordPressOrgNetwork solely handles WordPressOrgRequest, and it doesn't use RequestConverter.

TIL, thanks for the explanation. Now I have another question 😅 what are the differences between WordPressOrgRequest and RESTRequest? from the asURLRequest implementation in both structs, the way it sets the URL feels pretty similar 🤔

WordPressOrgRequest talks to WordPress in general. https://developer.wordpress.org/rest-api/

RESTRequest talks to WooCommerce plugin. https://woocommerce.com/document/woocommerce-rest-api/

WordPressOrgRequest talks to WordPress in general. https://developer.wordpress.org/rest-api/ RESTRequest talks to WooCommerce plugin. https://woocommerce.com/document/woocommerce-rest-api/

Just chiming in to clarify: I believe RESTRequest can be used to handle requests to all WordPress org and Woo requests. The difference between these two types is the way authentication is handled:

WordPressOrgRequest uses cookie authentication.

RESTRequest uses application password.
We should probably come up with better naming for these and update the documentation for clearer explanations.

Thanks for the details, Huong!

RESTRequest has this extra parameter/value let wooApiVersion: WooAPIVersion when compared to WordPressOrgRequest.
I thought the wooApiVersion parameter makes it clear that we should use RESTRequest for the WooCommerce plugin's REST API-related requests. I looked again now and noticed that WooAPIVersion has a case none which is an empty string.

We should probably come up with better naming for these and update the documentation for clearer explanations.

Maybe we could have a single struct instead of two and use the WooAPIVersion.none case. What do you think?

I will create an issue after we finish discussing it here.

Maybe we could have a single struct instead of two and use the WooAPIVersion.none case. What do you think?

This makes sense to me! A few things I notice:

We're using WordPressApiValidator in WordPressOrgRequest but this is not necessary. Since WordPressOrgRequest is required to conform to Request, we can use PlaceholderDataValidator instead.

WordPressOrgRequest is not using JSON encoding for POST and PUT requests like RESTRequest does, so unifying them with RESTRequest implementation will fix this too.

Let's use RESTRequest for both then.

Created an issue here #8512

Networking/Networking/Requests/AuthenticatedRequest.swift

jaclync · 2022-12-29T03:46:01Z

Networking/Networking/Network/AlamofireNetwork.swift

-                    }
-            case .failure(let error):
-                completion(nil, error)
+        let request = requestConverter.convert(request)


I see, now I understand why it has to be converted here - URLRequestConvertible is only available here and not after sessionManager.request(request) where the retry takes place, and the conversion requires the site URL from the credentials so it's not possible at the JetpackRequest call sites. I can't think of other solutions given these constraints, the only thought I had is to move the authentication step to the URLRequestConvertible level but I'm not sure if that works with the retry and what's the best practice.

Networking/Networking/ApplicationPassword/RequestProcessor.swift

…and application password authentication methods.

selanthiraiyan · 2022-12-29T11:03:49Z

From the wp-admin UI here, it looks like it shows the name of the app with a CTA to revoke. If the user chooses to revoke the application password here, it feels like they intentionally want to remove access from the app? If that's the case, then it feels a bit intrusive how the app just creates another password. I'm sure this has been thought out before, just sharing my observations from the review.

I started an internal discussion about this, and we will update this if we decide to follow a different flow. p1672283352691059-slack-C046HDZL87J

selanthiraiyan · 2022-12-29T11:10:17Z

Thanks, @jaclync. I addressed your comments. Kindly take another look when you can. 🙏

jaclync

Thanks for all the updates and clarifications, LGTM now 👍 sorry it took a while, since the changes are now 1000+ 😆

One non-blocking suggestion is to unit test RequestProcessor on the retry and adaption behavior. given the current PR size, feel free to consider it for the future.

jaclync · 2022-12-29T13:18:18Z

Networking/Networking/ApplicationPassword/RequestAuthenticator.swift

+                return try? DefaultApplicationPasswordUseCase(username: username,
+                                                              password: password,
+                                                              siteAddress: siteAddress)


does it make sense to validate these two URLs during the login step so that we don't have to handle this error scenario here? If the URLs are invalid from the beginning, the app doesn't seem usable since it can never generate an application password.

jaclync · 2022-12-29T13:28:36Z

docs/architecture-overview.md

+3.  **RESTRequest**
+
+        Represents a REST API request which will be used to contact to the site directly. (Skipping Jetpack tunnel) 
+        These requests are authenticated using application passwords. 


nit: maybe can mention AuthenticatedRESTRequest here?

Suggested change

These requests are authenticated using application passwords.

These requests are then authenticated using an application password using `AuthenticatedRESTRequest`.

Done in 9464bd1

jaclync · 2022-12-29T13:30:43Z

docs/architecture-overview.md

+        Injects application password and a custom user-agent header into anything that conforms to the URLConvertible protocol. Usually wraps up
+        a RESTRequest.


nit: "anything that conforms to the URLConvertible protocol" might not hold anymore?

Suggested change

Injects application password and a custom user-agent header into anything that conforms to the URLConvertible protocol. Usually wraps up

a RESTRequest.

Injects application password and a custom user-agent header into anything that conforms to the URLConvertible protocol and can be authenticated with an application password. For example, a RESTRequest.

Done in 9464bd1

jaclync · 2022-12-29T13:43:06Z

Networking/NetworkingTests/Network/RequestConverterTests.swift

+    func test_jetpack_request_is_returned_when_credentials_not_available() {
+        // Given
+        let converter = RequestConverter(credentials: nil)
+        let jetpackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test", availableAsRESTRequest: false)


nit: does it make sense to set availableAsRESTRequest to true to enable this conversion in any case?

Done in d0b7993

jaclync · 2022-12-29T13:47:52Z

Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift

+        let request = try converter.convert(jetpackRequest).asURLRequest()
+        let updatedRequest = try authenticator.authenticate(request)


nit: is the conversion necessary since availableAsRESTRequest is false already and RequestConverter has been tested separately?

Done in 7ccf5a4

jaclync · 2022-12-29T13:48:35Z

Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift

+        let updatedRequest = try authenticator.authenticate(request)

        // Then
-        XCTAssertTrue(updatedRequest is UnauthenticatedRequest)


nit: does it make sense to recover this assertion? or has the type of request changed?

We will be working with URLRequest here. So I had to remove the type checking the assertion and check the header fields directly instead.

jaclync · 2022-12-29T13:51:34Z

Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift

-        authenticator.authenticateRequest(request) { result in
-            updatedRequest = try? result.get()
-        }
+        let request = try converter.convert(jetpackRequest).asURLRequest()


nit: similar to before, the request can be passed directly (JetpackRequest or RESTRequest) without the converter so that this is testing just one thing (RequestAuthenticator)

Same for other test cases in this file.

Great point. Done in 7ccf5a4

jaclync · 2022-12-29T14:01:39Z

Networking/NetworkingTests/Requests/AuthenticatedDotcomRequestTests.swift

+        guard case let .wpcom(_, authToken, _) = credentials else {
+            XCTFail("Missing credentials.")
+            return
+        }
+


nit: do we still need Credentials? it looks like we can just pass an auth token string to the next line directly.

Same for other test cases in this file.

Done in c66416d

…nly authentication related logic.

…y unit tests.

selanthiraiyan · 2022-12-30T02:31:09Z

Thank you for your patience and the detailed review, Jaclyn! ❤️ 🙇
I have addressed your recent comments, and I will go ahead and merge this now. Kindly take another look if you get the time. 🙏

One non-blocking suggestion is to unit test RequestProcessor on the retry and adaption behavior. given the current PR size, feel free to consider it for the future.

I created an issue for this. #8507

selanthiraiyan added 11 commits December 26, 2022 09:53

Make Jetpack tunnel requests in CouponsRemote available for REST AP…

04541f7

…I using application password.

Use JSON encoding for post and put requests.

6e8efb9

Remove comment about fallbackRequest parameter which no longer exists.

5b00fce

Add String+URL with trimSlahses helper method.

e8189f7

Make AuthenticatedRequest and UnauthenticatedRequest work with `U…

fd5156f

…RLRequest` instead of `URLRequestConvertible`.

RequestConverter for converting Jetpack tunnel requests into REST r…

81731b0

…equests if applicable.

RequestProcessor to authenticate and retry alamofire network requests.

1068172

Stop handling authentication in RESTRequest class.

dc94a9a

RequestAuthenticator for authenticating network requests.

c243504

Change AlamofireNetwork to use request adapter and retrier for auth…

d1b7b55

…entication.

Revert the changes made to CouponsRemote to make the requests avail…

87752cd

…able for REST API.

selanthiraiyan added type: task An internally driven task. feature: login Related to any part of the log in or sign in flow, or authentication. labels Dec 27, 2022

Revert "Use JSON encoding for post and put requests."

8431361

This reverts commit 6e8efb9.

selanthiraiyan mentioned this pull request Dec 27, 2022

[REST API] Coupons - Make Jetpack requests available as REST API requests. #8477

Closed

1 task

selanthiraiyan requested review from itsmeichigo and jaclync and removed request for itsmeichigo December 27, 2022 08:50

selanthiraiyan added this to the 11.8 milestone Dec 27, 2022

selanthiraiyan added 5 commits December 27, 2022 18:49

Add headers to users_me mock json.

5e35c3e

Fix the missing comma in json.

cf8a394

Add headers for order related mock json files.

0e16a10

Add headers for UI testing related mock json files.

5262ec3

Fix JSON format error in mock json file.

504fcf6

selanthiraiyan requested a review from itsmeichigo December 28, 2022 00:36

selanthiraiyan marked this pull request as ready for review December 28, 2022 01:02

selanthiraiyan requested a review from a team as a code owner December 28, 2022 01:02

jaclync self-assigned this Dec 28, 2022

jaclync reviewed Dec 28, 2022

View reviewed changes

selanthiraiyan added 2 commits December 28, 2022 16:37

Revert "Fix the missing comma in json."

b77161c

This reverts commit cf8a394.

Revert "Add headers to users_me mock json."

9dcee6d

This reverts commit 5e35c3e.

selanthiraiyan added 7 commits December 29, 2022 06:06

Move header setting code to AuthenticatedRequest.

5965160

Add comment to explain why only RESTRequest needs to call `validate…

f75af92

…()`.

Fix spacing violation.

7f17011

Remove unnecessary await call.

f9f0c74

Rename error to explain the scenario.

ae5b15c

Trim comment for brevity.

7433ce6

Add tests to ensure that Jetpack request is converted into REST API r…

654b529

…equest properly.

selanthiraiyan requested a review from jaclync December 29, 2022 01:55

Remove unused authenticateRequest method.

3052048

jaclync reviewed Dec 29, 2022

View reviewed changes

selanthiraiyan added 3 commits December 29, 2022 12:01

Add RESTRequest to the documentation.

f1511f8

Simply let unwrap statement.

a3e21d8

Break AuthenticatedRequest into two parts for handling WPCOM token …

e30bfd5

…and application password authentication methods.

selanthiraiyan requested a review from jaclync December 29, 2022 11:10

jaclync approved these changes Dec 29, 2022

View reviewed changes

selanthiraiyan mentioned this pull request Dec 30, 2022

[REST API] Logout if application password regeneration fails #8506

Closed

selanthiraiyan added 4 commits December 30, 2022 07:11

Update docs about authenticated requests.

9464bd1

Enable availableAsRESTRequest to allow REST request conversion.

d0b7993

Remove RequestConverter and make RequestAuthenticatorTests test o…

7ccf5a4

…nly authentication related logic.

Use authentication token directly instead of Credentials to simplif…

c66416d

…y unit tests.

selanthiraiyan merged commit fe3f198 into trunk Dec 30, 2022

selanthiraiyan deleted the feat/8482-alamofire-retry-adapt-request branch December 30, 2022 02:32

	These requests are authenticated using application passwords.
	These requests are then authenticated using an application password using `AuthenticatedRESTRequest`.

		Injects application password and a custom user-agent header into anything that conforms to the URLConvertible protocol. Usually wraps up
		a RESTRequest.

		let request = try converter.convert(jetpackRequest).asURLRequest()
		let updatedRequest = try authenticator.authenticate(request)

[REST API] Alamofire network retry and adapt network requests #8483

[REST API] Alamofire network retry and adapt network requests #8483

Uh oh!

Conversation

selanthiraiyan commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

UI tests

Testing instructions

Screenshots

Uh oh!

wpmobilebot commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

peril-woocommerce bot commented Dec 28, 2022

Uh oh!

selanthiraiyan commented Dec 29, 2022

Uh oh!

jaclync commented Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

selanthiraiyan commented Dec 29, 2022

Uh oh!

selanthiraiyan commented Dec 29, 2022

Uh oh!

jaclync left a comment

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!

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!

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!

Choose a reason for hiding this comment

selanthiraiyan commented Dec 27, 2022 •

edited

Loading

wpmobilebot commented Dec 27, 2022 •

edited

Loading

jaclync commented Dec 29, 2022 •

edited

Loading