REST API: Update `AlamofireNetwork` to support authentication with application passwords #8402

itsmeichigo · 2022-12-14T05:03:38Z

Closes: #8389

Description

This PR updates the Networking layer to support authentication with application passwords:

Created new request type RESTRequest. This requires a site URL to construct the endpoint URL for its path. It also accepts an optional fallback Jetpack request to be sent if application password is blocked for the given site.
Created a helper class RequestAuthenticator to update requests with the appropriate Authorization header if possible. This class handles fetching application password for REST requests and wrapping other request types with WPCOM tokens.
Updated AlamofireNetwork with a request authenticator to authenticate requests before sending them.
~~Updated DefaultStoresManager and AuthenticatedState to send an updated site ID to AlamofireNetwork when the selected store changes - to update the application password use case accordingly.~~

Testing instructions

No API endpoint has been migrated to use REST API yet, but please run the app and do a smoke test to make sure that API requests still work as before.

Screenshots

N/A

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

wpmobilebot · 2022-12-14T05:18:12Z

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 pr8402-e66619a on your iPhone

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

…ation password is blocked

…icatedState" This reverts commit 17cae6a.

selanthiraiyan · 2022-12-15T03:38:27Z

Networking/Networking/Network/AlamofireNetwork.swift

+            return
+        }
+        let applicationPasswordUseCase = TemporaryApplicationPasswordUseCase(siteID: siteID, credentials: credentials)
+        requestAuthenticator.updateApplicationPasswordHandler(with: applicationPasswordUseCase)


Nit: What do you think about just passing the siteID and let RequestAuthenticator take care of creating a new instance of the ApplicationPasswordUseCase?

I propose this because,

RequestAuthenticator owns the usecase and we can also let it create the usecase.

AlamofireNetwork doesn't have to know about the application passwords at all.

If we do this change we can also rename the methods func configureApplicationPasswordHandler and updateApplicationPasswordHandler accordingly. (Maybe just updateSiteID)

Yup, that was what I followed initially - but then I found that injecting the use case makes RequestAuthenticator more testable. Otherwise, I'm not sure how to test it with a mock use case. Please let me know if you have any suggestions regarding testability without an injected use case.

Oh, that is a good point.

Can we use MockNetwork to mock the responses for unit testing? i.e. Inject a network into RequestAuthenticator and use that to initialize ApplicationPasswordUseCase

selanthiraiyan · 2022-12-15T03:43:14Z

Networking/Networking/Requests/RESTRequest.swift

+extension RESTRequest {
+    /// Updates the request headers with authentication information.
+    ///
+    func updateRequest(with applicationPassword: ApplicationPassword) throws -> URLRequest {


Super nit: I would prefer the name authenticateRequest(with to make the work of this method clear.

Updated as part of 3ede699.

selanthiraiyan · 2022-12-15T03:44:15Z

Networking/Networking/Requests/RESTRequest.swift

+        let username = "username"
+        let password = "password"


Super nit: Would it make sense to move the strings from this method to a private constants enum?

This is actually incorrect, I meant to get the username and password from application password instead 😅. Updated in 3ede699.

selanthiraiyan · 2022-12-15T03:46:40Z

WooCommerce/Classes/Yosemite/DefaultStoresManager.swift

        restoreSessionSiteIfPossible()
        ServiceLocator.pushNotesManager.reloadBadgeCount()

+        if let state = self.state as? AuthenticatedState {


Nit: Is self needed here?

I think we need self, because I want to reference the state property of the class.

selanthiraiyan · 2022-12-15T03:56:59Z

Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift

+import Alamofire
+@testable import Networking
+
+final class RequestAuthenticatorTests: XCTestCase {


Nice tests. 👏

selanthiraiyan · 2022-12-15T03:57:59Z

Networking/Networking/Network/RequestAuthenticator.swift

+                    return try await useCase.generateNewPassword()
+                }()
+                try await MainActor.run {
+                    let updatedRequest = try restRequest.updateRequest(with: applicationPassword)


❓ Should the updateRequest call happen in the main thread as well?

Yes, I got a failure when I didn't call updateRequest in the main thread - this was because the method makes a call to UserAgent.defaultUserAgent, which creates a WKWebView and that requires main thread.

selanthiraiyan · 2022-12-15T03:59:39Z

Networking/Networking/Network/RequestAuthenticator.swift

+        Task(priority: .medium) {
+            do {
+                let applicationPassword: ApplicationPassword = try await {
+                    if let password = useCase.applicationPassword {
+                        return password
+                    }
+                    return try await useCase.generateNewPassword()
+                }()
+                try await MainActor.run {
+                    let updatedRequest = try restRequest.updateRequest(with: applicationPassword)
+                    completion(updatedRequest)
+                }
+            } catch {
+                DDLogWarn("⚠️ Error generating application password and update request: \(error)")
+                // TODO: add Tracks
+                // Get the fallback Jetpack request to handle if possible.
+                let fallbackRequest = restRequest.fallbackRequest ?? request
+                await MainActor.run {
+                    completion(createAuthenticatedRequestIfPossible(for: fallbackRequest))
+                }
+            }
+        }


Super nit: What do you think about moving this to a private method named handleRESTReqest?

Updated in d1ea84c.

selanthiraiyan · 2022-12-15T04:01:01Z

Networking/Networking/Network/RequestAuthenticator.swift

+
+    /// Attempts creating a request with WPCOM token if possible.
+    ///
+    private func createAuthenticatedRequestIfPossible(for request: URLRequestConvertible) -> URLRequestConvertible {


Super nit: Consider naming it something like authenticateUsingWPCOMTokenIfPossible.

Also updated in d1ea84c.

selanthiraiyan · 2022-12-15T04:01:54Z

Networking/Networking/Network/RequestAuthenticator.swift

+
+/// Helper class to update requests with authorization header if possible.
+///
+final class RequestAuthenticator {


Clever work extracting this into a separate class. 👏

…annot be generated

… the initializer

itsmeichigo · 2022-12-21T05:54:55Z

@selanthiraiyan I've updated this PR following our latest approach to use application password with cookie authentication and no fallback if the application password cannot be fetched. Please take another look, thanks!

selanthiraiyan · 2022-12-21T06:43:07Z

Networking/Networking/Network/AlamofireNetwork.swift

-                upload.responseData { response in
-                    completion(response.value, response.error)
+        requestAuthenticator.authenticateRequest(request) { [weak self] result in
+            guard let self else { return }


I think we should call completion with an error when self isn't available.

Updated in aa6d8e9.

selanthiraiyan · 2022-12-21T06:44:16Z

Networking/Networking/Network/RequestAuthenticator.swift

+        let useCase: ApplicationPasswordUseCase? = {
+            if let applicationPasswordUseCase {
+                return applicationPasswordUseCase
+            } else if let credentials,


Nit: We can skip else if let credentials and directly write else if case let .wporg(username, password, siteAddress) = credentials

Updated in 67b9316.

selanthiraiyan · 2022-12-21T06:46:02Z

Networking/Networking/Network/RequestAuthenticator.swift

+        guard let jetpackRequest = request as? JetpackRequest,
+              jetpackRequest.availableAsRESTRequest,
+              let useCase = applicationPasswordUseCase,
+              case let .some(.wporg(_, _, siteAddress)) = credentials else {


Do we need some here? Can we write this as case let .wporg(_, _, siteAddress) = credentials

Also updated in 67b9316.

selanthiraiyan · 2022-12-21T06:52:45Z

Networking/Networking/Requests/JetpackRequest.swift

        return DotcomValidator()
    }
+
+    func createRESTRequest(with siteURL: String) -> RESTRequest {


Nits:

For consistency I would follow asURLRequest and use asRESTRequest as name.

We could return optional here and make availableAsRESTRequest property private. i.e. Check availableAsRESTRequest and return nil if false.

Good point 👍 Updated in e66619a.

peril-woocommerce · 2022-12-21T08:27:52Z

	Warnings
⚠️	PR is not assigned to a milestone.

Generated by 🚫 dangerJS

itsmeichigo added 6 commits December 13, 2022 12:06

Inject siteID from DefaultStoresManager to AlamofireNetwork

7895ade

Add new request type for REST API requests

0099895

Configure application password use case in AlamofireNetwork

3efefba

Add new method to update headers for REST requests

f4e1519

Update AlamofireNetwork to handle application password

823607a

Use Task only for fetching application password

583d756

itsmeichigo added the type: task An internally driven task. label Dec 14, 2022

itsmeichigo added 10 commits December 14, 2022 12:41

Add a fallback Jetpack request to REST request to trigger when applic…

763ba7f

…ation password is blocked

Fix line limit violation

168a6ac

Update comment for RESTRequest initializer

9b0d019

Inject the complete use case to AlamofireNetwork from AuthenticatedState

17cae6a

Update comments for AlamofireNetwork

5e07911

Revert "Inject the complete use case to AlamofireNetwork from Authent…

85c31f8

…icatedState" This reverts commit 17cae6a.

Separate authentication logic to a new class

9740cbb

Make ApplicationPasswordUseCase injectable

cc87a50

Add unit tests for RequestAuthenticator

ad69556

Update comments for RESTRequest

4b5423e

itsmeichigo requested review from jaclync and selanthiraiyan December 14, 2022 10:53

selanthiraiyan self-assigned this Dec 15, 2022

selanthiraiyan reviewed Dec 15, 2022

View reviewed changes

itsmeichigo added 2 commits December 15, 2022 11:16

Fix incorrect username and password when authenticating rest request

3ede699

Reformat code for RequestAuthenticator

d1ea84c

itsmeichigo mentioned this pull request Dec 15, 2022

REST API: Update coupon mappers to support REST API #8416

Closed

1 task

itsmeichigo closed this Dec 16, 2022

itsmeichigo reopened this Dec 16, 2022

itsmeichigo added 3 commits December 16, 2022 15:52

Remove fallback request and header from

139348b

Update JetpackRequest to be able to converted to a REST request

b7942a5

Update RequestAuthenticator to return error if application password c…

63a76f6

…annot be generated

itsmeichigo added 9 commits December 16, 2022 16:45

Revert changes to AuthenticatedState and DefaultStoresManager

012d6c8

Fix line length violation

f779ff2

Keep network local in AuthenticatedState intializer

c87f274

Merge branch 'trunk' into feat/8389-network-update-for-rest-api

1ef0e5b

Update unit tests for RequestAuthenticator

fdf9566

Merge branch 'trunk' into feat/8389-network-update-for-rest-api

498cfe8

Update RequestAuthenticator to set up application password usecase in…

ab3a53b

… the initializer

Merge branch 'trunk' into feat/8389-network-update-for-rest-api

55d4fc5

Create application password use case in RequestAuthenticator

f3fa7aa

itsmeichigo marked this pull request as ready for review December 21, 2022 05:53

itsmeichigo requested a review from selanthiraiyan December 21, 2022 05:55

selanthiraiyan approved these changes Dec 21, 2022

View reviewed changes

itsmeichigo added 3 commits December 21, 2022 15:23

Return completion when self is not found

aa6d8e9

Simplify condition checks for enum case

67b9316

Rename createRESTRequest to asRESTRequest

e66619a

itsmeichigo added this to the 11.8 milestone Dec 21, 2022

itsmeichigo enabled auto-merge December 21, 2022 08:29

itsmeichigo merged commit fd4ec3a into trunk Dec 21, 2022

itsmeichigo deleted the feat/8389-network-update-for-rest-api branch December 21, 2022 08:55

REST API: Update AlamofireNetwork to support authentication with application passwords #8402

REST API: Update AlamofireNetwork to support authentication with application passwords #8402

Uh oh!

Conversation

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

Uh oh!

Description

Testing instructions

Screenshots

Uh oh!

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

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!

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!

itsmeichigo commented Dec 21, 2022

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!

peril-woocommerce bot commented Dec 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

REST API: Update `AlamofireNetwork` to support authentication with application passwords #8402

REST API: Update `AlamofireNetwork` to support authentication with application passwords #8402

itsmeichigo commented Dec 14, 2022 •

edited

Loading

wpmobilebot commented Dec 14, 2022 •

edited

Loading