[REST API] Alamofire network retry and adapt network requests #8483
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| enum RequestAuthenticatorError: Error { | ||
| case applicationPasswordUseCaseNotAvailable | ||
| case applicationPasswordNotAvailable | ||
| } | ||
|
|
||
| /// Authenticates request | ||
| /// | ||
| public struct RequestAuthenticator { | ||
| /// Credentials. | ||
| /// | ||
| let credentials: Credentials? | ||
|
|
||
| /// The use case to handle authentication with application passwords. | ||
| /// | ||
| private let applicationPasswordUseCase: ApplicationPasswordUseCase? | ||
|
|
||
| /// Sets up the authenticator with optional credentials and application password use case. | ||
| /// `applicationPasswordUseCase` can be injected for unit tests. | ||
| /// | ||
| init(credentials: Credentials?, applicationPasswordUseCase: ApplicationPasswordUseCase? = nil) { | ||
| self.credentials = credentials | ||
| let useCase: ApplicationPasswordUseCase? = { | ||
| if let applicationPasswordUseCase { | ||
| return applicationPasswordUseCase | ||
| } else if case let .wporg(username, password, siteAddress) = credentials { | ||
| return try? DefaultApplicationPasswordUseCase(username: username, | ||
| password: password, | ||
| siteAddress: siteAddress) | ||
| } else { | ||
| return nil | ||
| } | ||
| }() | ||
| self.applicationPasswordUseCase = useCase | ||
| } | ||
|
|
||
| func authenticate(_ urlRequest: URLRequest) throws -> URLRequest { | ||
| if isRestAPIRequest(urlRequest) { | ||
| return try authenticateUsingApplicationPasswordIfPossible(urlRequest) | ||
| } else { | ||
| return try authenticateUsingWPCOMTokenIfPossible(urlRequest) | ||
| } | ||
| } | ||
|
|
||
| func generateApplicationPassword() async throws { | ||
| guard let applicationPasswordUseCase = applicationPasswordUseCase else { | ||
jaclync marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| throw RequestAuthenticatorError.applicationPasswordUseCaseNotAvailable | ||
| } | ||
| let _ = try await applicationPasswordUseCase.generateNewPassword() | ||
| return | ||
| } | ||
|
|
||
| /// Checks whether the given URLRequest is eligible for retyring | ||
| /// | ||
| func shouldRetry(_ urlRequest: URLRequest) -> Bool { | ||
| isRestAPIRequest(urlRequest) | ||
| } | ||
| } | ||
|
|
||
| private extension RequestAuthenticator { | ||
| /// To check whether the given URLRequest is a REST API request | ||
| /// | ||
| func isRestAPIRequest(_ urlRequest: URLRequest) -> Bool { | ||
| guard case let .wporg(_, _, siteAddress) = credentials, | ||
| let url = urlRequest.url, | ||
| url.absoluteString.hasPrefix(siteAddress.trimSlashes() + "/" + RESTRequest.Settings.basePath) else { | ||
| return false | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| /// Attempts creating a request with WPCOM token if possible. | ||
| /// | ||
| func authenticateUsingWPCOMTokenIfPossible(_ urlRequest: URLRequest) throws -> URLRequest { | ||
| guard case let .wpcom(_, authToken, _) = credentials else { | ||
| return UnauthenticatedRequest(request: urlRequest).asURLRequest() | ||
| } | ||
|
|
||
| return AuthenticatedRequest(authToken: authToken, request: urlRequest).asURLRequest() | ||
| } | ||
|
|
||
| /// Attempts creating a request with application password if possible. | ||
| /// | ||
| func authenticateUsingApplicationPasswordIfPossible(_ urlRequest: URLRequest) throws -> URLRequest { | ||
| guard let applicationPassword = applicationPasswordUseCase?.applicationPassword else { | ||
| throw RequestAuthenticatorError.applicationPasswordNotAvailable | ||
| } | ||
|
|
||
| return AuthenticatedRequest(applicationPassword: applicationPassword, request: urlRequest).asURLRequest() | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import Alamofire | ||
|
|
||
| /// Converter to convert Jetpack tunnel requests into REST API requests if needed | ||
| /// | ||
| struct RequestConverter { | ||
| let credentials: Credentials? | ||
|
|
||
| func convert(_ request: URLRequestConvertible) -> URLRequestConvertible { | ||
| guard let jetpackRequest = request as? JetpackRequest, | ||
|
Contributor
There was a problem hiding this comment. just wondering, do we only do the conversation from
Contributor
Author
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment. TIL, thanks for the explanation. Now I have another question 😅 what are the differences between
Contributor
Author
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment. Just chiming in to clarify: I believe
Contributor
Author
There was a problem hiding this comment. Thanks for the details, Huong!
Maybe we could have a single I will create an issue after we finish discussing it here.
Contributor
There was a problem hiding this comment.
This makes sense to me! A few things I notice:
Let's use |
||
| case let .wporg(_, _, siteAddress) = credentials, | ||
| let restRequest = jetpackRequest.asRESTRequest(with: siteAddress) else { | ||
| return request | ||
| } | ||
|
|
||
| return restRequest | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly related to this PR: I took a look at what might cause the use case to throw an error, and it looks like it could be from an invalid site address
ApplicationPasswordUseCaseError.invalidSiteAddress. Can we validate the site address when the user signs in with site credentials so that the use case can be non-nil here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
DefaultApplicationPasswordUseCaseinitialization can throw an error when we are not able to construct login/admin URL using the site address, hereI have renamed the error in this commit to explain the scenario better. ae5b15c
Sorry I don't understand this. Could you kindly explain how we can improve it? Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the login flow where the user signs in with site credentials, do we check if the login and admin URLs are valid? If so, maybe we don't need to check again so that the initialization always succeeds. I assume the API requests here are after the login flow, please correct me if this isn't the case.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We validate
loginURLandadminURLonly while sendingWordPressOrgNetwork, which currently happens during Jetpack setup here. This validation doesn't happen during site address login step. So we need to validate theloginURLandadminURLhere again.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Created an issue #8506