[Login] Improve error messaging displaying a new error screen when application password is disabled #15031

pmusolino · 2025-01-31T15:14:27Z

Closes: #14004 (and peaMlT-U4)

Description

This pull request introduces improvements to handling the scenario when application passwords are disabled in user's website. The changes involve updating the error handling UI and navigation logic to provide a better user experience. This is a continuation of the job started by @hafizrahman here.

Testing information

Create test site with WooCommerce and no Jetpack, on Jurassic Ninja.
Install and active a plugin like this one https://wordpress.org/plugins/disable-application-passwords/
Try to log in using application password to the test site.
Confirm that the new dialog is shown.
Confirm that tapping "Retry" button returns to two screens before the current one (instructions on how to approve the connection with application password).
Confirm that tapping "Log In With Another Account" returns to the initial screen of the login process.

Screenshots

Before	After

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

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

…ed application password.

…abled-note

…ledViewModel.swift - Modify navigation logic to pop to the third last view controller or root if fewer than three - Update primary button title to "Retry" for retrying application password authorization - Remove outdated primary button title "Log in with WordPress.com"

…ordDisabledViewModel` using Woo mobile guidelines

…swordViewModelTests`

…e else

wpmobilebot · 2025-01-31T15:33:20Z

📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS
	Build Number	pr15031-2c7f417
	Version	21.6
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`2c7f417`
	App Center Build	WooCommerce - Prototype Builds #12866

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

itsmeichigo

While testing navigation back and forth, I encountered a case where the application password fetch failed with the following log:

Tracked application_password_authorization_url_fetch_failed, properties: [error_domain: Networking.RequestAuthenticatorError, error_description: Alamofire.AFError.requestAdaptationFailed(error: Networking.RequestAuthenticatorError.applicationPasswordNotAvailable), error_code: 1]

In this case, the fetchAuthURL method failed with error and the logic went through to the catch block, which showed an alert instead of the application password not available screen:

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-02-03.at.11.50.29.mp4

We should also check the logic in this catch block to show the new screen if we get the applicationPasswordNotAvailable error.

itsmeichigo · 2025-02-03T05:01:51Z

.../Authentication/Application Password/ApplicationPasswordAuthorizationWebViewController.swift

+        // Pop back two view controllers to remove both error and web view
+        navigationController?.popViewController(animated: false)
+        navigationController?.popViewController(animated: true)


I think this is hacky and prone to error. How about looking for the index of ApplicationPasswordTutorialViewController in the navigation stack, and pop to the index before that?

A better option would be to send the view controller before the application password flow to this view controller via presentApplicationPasswordWebView in AuthenticationManager.

I agree with you and with your second suggestion, so I applied the new changes in this commit: 9b480ba

.../Authentication/Application Password/ApplicationPasswordAuthorizationWebViewController.swift

itsmeichigo · 2025-02-03T05:09:30Z

...erce/Classes/Authentication/Navigation Exceptions/ApplicationPasswordDisabledViewModel.swift

+        if viewControllers.count >= 3 {
+            navigationController.popToViewController(viewControllers[viewControllers.count - 3], animated: true)
+        } else {
+            navigationController.popToRootViewController(animated: true)
        }


The number 3 seems to be a magic number, it's not clear why we have to do this. Please consider my suggestion above about looking for the index of a specific view controller to pop to.

Now I'm using the previousVC like in the comment above, please feel free to check changes here: 9b480ba 🙏

…abled-note-take-2

- Modify `ApplicationPasswordAuthorizationWebViewController.swift` to include a reference to the previous view controller for navigation purposes. - Update `AuthenticationManager.swift` to pass the previous view controller to the web view used for application password authorization. - Enhance `ApplicationPasswordDisabledViewModel.swift` to store the previous view controller and use it for navigation. - Adjust `PostSiteCredentialLoginChecker.swift` to capture and utilize the last view controller before the application password flow. - Update tests in `ApplicationPasswordDisabledViewModelTests.swift` to include the previous view controller in the view model initialization.

…abled-note-take-2

pmusolino · 2025-02-03T14:54:44Z

@itsmeichigo this is ready for another check 🙌

itsmeichigo

Thanks for the updates, Paolo. I found a few issues with the new changes in the comments below.

Also, it seems that you missed the comment in my previous round about missing the case when fetching the authentication URL fails with Alamofire.AFError.requestAdaptationFailed(error: Networking.RequestAuthenticatorError.applicationPasswordNotAvailable).

.../Authentication/Application Password/ApplicationPasswordAuthorizationWebViewController.swift

...erce/Classes/Authentication/Navigation Exceptions/ApplicationPasswordDisabledViewModel.swift

itsmeichigo · 2025-02-04T04:25:43Z

WooCommerce/Classes/Authentication/PostSiteCredentialLoginChecker.swift

+                    // show application password disabled error, and catch the last view controller that was showing before the Application Password flow.
+                    let previousViewController = navigationController.viewControllers.dropLast().last


What is the exact controller that you are navigating to here? Please note that the PostSiteCredentialLoginChecker can be triggered from two places: (1) after site credential login (if application password is not required), and (2) after application password login.

For (1), I'd suggest sending nil as the previous view controller to the PostSiteCredentialLoginChecker, so that the error page would simply pop itself upon navigating back.

For (2), I'd suggest sending the previous view controller from the application password entry point in AuthenticationManager

Thank you for your suggestions @itsmeichigo, they are gold and I wasn't 100% aware of the flow. Can you check if the changes make sense now, please? 6bdb62d

itsmeichigo · 2025-02-04T04:28:32Z

...erce/Classes/Authentication/Navigation Exceptions/ApplicationPasswordDisabledViewModel.swift

+        if let previousViewController = previousViewController {
+            navigationController.popToViewController(previousViewController, animated: true)
+        } else {
+            navigationController.popToRootViewController(animated: true)


Since you want to pop to the previous view controller, popToRoot seems incorrect. Should it be popViewController?

When I implemented it, during my testing process (at least in every test I did), the previous view controller immediately before the error screen was the loading screen—the one with just a loader in the middle of the view. If we pop back to it (by calling popViewController), the user would remain stuck there, so we need to go back further to ensure a better user experience.

Thanks for the explanation. From my understanding, you already sent the previousViewController for the application password flow, so this should not happen. Also, you are sending nil for the site credential login flow, so tapping "Try Again" should navigate back to the login screen instead of the prologue screen IMO. Is there any other edge case we should be aware of?

Yes, in theory, there are no other edge cases and I didn't find any other specific edge cases so I'm putting popViewController. 👍

…ationPasswordAuthorizationWebViewController.swift Co-authored-by: Huong Do <[email protected]>

…cationPasswordDisabledViewModel.swift Co-authored-by: Huong Do <[email protected]>

- Update `AuthenticationManager.swift` to include `previousViewController` parameter in `checkSiteCredentialLogin` function - Modify `PostSiteCredentialLoginChecker.swift` to accept `previousViewController` in the initializer and utilize it in error handling

…rTests: update `PostSiteCredentialLoginChecker` instantiation in `PostSiteCredentialLoginCheckerTests.swift` to include `previousViewController: nil` across multiple test cases.

pmusolino · 2025-02-04T13:02:56Z

Also, it seems that you missed the comment in my previous round about missing the case when fetching the authentication URL fails with Alamofire.AFError.requestAdaptationFailed(error: Networking.RequestAuthenticatorError.applicationPasswordNotAvailable).

I'm sorry for missing that. How is it possible to generate that specific error? Is this something you simulate by editing the code, or is there a specific case that you can consistently replicate on JN sites? Note that simply disabling application passwords doesn't generate that error for me on my side.

itsmeichigo · 2025-02-05T04:04:50Z

I'm sorry for missing that. How is it possible to generate that specific error? Is this something you simulate by editing the code, or is there a specific case that you can consistently replicate on JN sites? Note that simply disabling application passwords doesn't generate that error for me on my side.

As stated above, I encountered this by navigating back and forth from the application password disabled error screen. Specifically:

Follow the steps to the application password login flow until the disabled error screen.
Tap Retry or "<" to retry login.
If the same disabled screen is display, try again.
After a few times, I got the error Alamofire.AFError.requestAdaptationFailed(error: Networking.RequestAuthenticatorError.applicationPasswordNotAvailable) for the URL fetching request, and an alert is displayed as the video above.

Though this sounds like an edge case, I believe it should be handled given 2 reasons:

The application password flow is unpredictable for different sites.
It makes sense to handle the same issue when the error clearly is applicationPasswordNotAvailable.

…abled-note-take-2

…ller`

…hange navigationController method from `popToRootViewController` to `popViewController`

pmusolino · 2025-02-05T15:37:59Z

@itsmeichigo I was able to replicate the issue you mentioned when going back and forth. Do you think the error should be handled with the same error screen, or should we show a different error? I'm not sure if the error is always due to the application password being disabled or something else, because if I enable the application password, replicating that specific error becomes impossible, at least in my tests. If we display an error regarding fetching the URL, it will be generic and not helpful for the user, so I think I can move forward by displaying the same error screen about the application password being disabled. What do you think?

…abled-note-take-2

itsmeichigo · 2025-02-06T03:44:42Z

@pmusolino I debugged the app and found that the flow now doesn't go through web application password, so we've been testing the wrong thing. Please note: you can only reach the web application password flow if you see the tutorial page before opening the web view.

I used Proxyman to check the requests and here are what I'm seeing with a JN site that has https://wordpress.org/plugins/disable-application-passwords/ installed, and this is the flow I see up until the error reported above:

So the flow first went through the application password retrieval with nonce (handled by DefaultApplicationPasswordUseCase in the app, then failed when the POST request rest_route=/wp/v2/users/me/application-passwords failed. The application password disabled screen displayed here was added by Hafiz, not the implementation added in this PR.

When I navigated back and forth many times to retry, I finally got the 429 error (too many requests) for the nonce-retrieval request (wp-admin/admin-ajax.php?action=rest-nonce). Only then did I got redirected to the web application password flow and saw the error reported above.

Now, to actually reach the application password web view screen, we need to block the programmatic site credential login through wp-login.php. There are a few ways to do this: customize the login URL for the wp-admin page, enable captcha on the test site, etc. I enabled captcha for my site's login page by adding WordFence plugin to my site and enabling reCAPTCHA to block auto-login.

As can be seen in the screencast below, after entering site credentials, I was redirected to the web application password tutorial page with the error at the top. Then when I proceeded, I immediately got the error alert for disabled application password. This means it's a must to display the disabled error screen upon catching error when fetching application password URL.

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-02-06.at.10.41.07.mp4

Regarding your question:

I'm not sure if the error is always due to the application password being disabled or something else, because if I enable the application password, replicating that specific error becomes impossible, at least in my tests

A few suggestions in case it's helpful:

Please ensure to understand the application password flow thoroughly by taking a closer look at the implementation.
Please be more proactive in debugging when you see something you cannot explain.

…ntroller` and display errorUI instead of error alert

pmusolino · 2025-02-07T00:36:06Z

Thank you so much Huong for the explanation.
I did another test and was able to replicate it, and I saw your same requests. I replaced the alert with the new UI error screen and tomorrow I'll check If I need to do other changes. In the meantime can you take a look?9fafc530cd0f85712d6e925be4c41ab80cca27a2

Extra:
While testing, I gained insights into how we're currently managing error codes related to application passwords. I've identified a scenario that may not be easily addressed and might have been discussed previously. In a new JN site, before encountering any 429 errors and with application passwords enabled, installing a plugin such as WPS Hide Login—which relocates the login page—causes the webpage to load but displays a "Page not found" message instead of an error (see image below).

I believe this happens because the handleSiteCredentialLoginFailure function in the AuthenticationManager returns a generic SiteCredentialLoginError. Although we can recognize this situation as inaccessibleLoginPage under the 404 error code, and we include a tutorial message stating, "This could be because your store has some extra security steps in place," we still direct the user to the webpage. This occurs because we can't distinguish this specific error from others that also return a 404 code. From a UI/UX perspective, this isn't ideal for the end user, and it remains a challenging issue to identify and resolve, especially if we have no other information on the page with which to differentiate an authentication page from an error page.

…abled-note-take-2

itsmeichigo · 2025-02-07T04:16:51Z

In a new JN site, before encountering any 429 errors and with application passwords enabled, installing a plugin such as WPS Hide Login—which relocates the login page—causes the webpage to load but displays a "Page not found" message instead of an error

Regarding this, I believe WPS Hide Login has the option to redirect to 404 immediately when accessing the wp-admin directory. The authentication URL belongs to this directory by default, hence the Page not found that you saw.

So, this is not an issue with the customization of the login page, but a custom rule of redirect to 404. In this case, I don't think we can do anything, users will need to contact support for help. cc @hichamboushaba in case he has some other suggestions.

…abled-note-take-2

pmusolino · 2025-02-07T11:51:47Z

Regarding this, I believe WPS Hide Login has the option to redirect to 404 immediately when accessing the wp-admin directory. The authentication URL belongs to this directory by default, hence the Page not found that you saw.

Yes, I think that it's a very common behavior for this type of plugin.

wpmobilebot · 2025-02-07T13:24:27Z

Version 21.7 has now entered code-freeze, so the milestone of this PR has been updated to 21.8.

hafizrahman and others added 7 commits October 31, 2024 16:23

First pass at converting error dialog into an error screen for disabl…

cf69dbb

…ed application password.

Merge branch 'trunk' into issue/14004-better-application-password-dis…

74f3268

…abled-note

update: localization keys and message formatting in `ApplicationPassw…

170d58e

…ordDisabledViewModel` using Woo mobile guidelines

update: authentication tests and localization keys in `ApplicationPas…

2887491

…swordViewModelTests`

clean: removed TODO comment because the track already happen somewher…

43e803d

…e else

update: release-notes 21.7

965c4d3

pmusolino added the feature: login Related to any part of the log in or sign in flow, or authentication. label Jan 31, 2025

pmusolino added this to the 21.7 milestone Jan 31, 2025

pmusolino requested review from selanthiraiyan, hichamboushaba and itsmeichigo January 31, 2025 15:14

pmusolino mentioned this pull request Jan 31, 2025

[Login] Improve error messaging when application password is disabled. #14269

Closed

4 tasks

update: added PR url in 21.7 release notes

58faf5e

itsmeichigo self-assigned this Feb 3, 2025

itsmeichigo reviewed Feb 3, 2025

View reviewed changes

pmusolino added 4 commits February 3, 2025 14:34

Merge branch 'trunk' into issue/14004-better-application-password-dis…

79d524a

…abled-note-take-2

fix: remove unused code

3f40c59

Merge branch 'trunk' into issue/14004-better-application-password-dis…

98810f3

…abled-note-take-2

pmusolino requested a review from itsmeichigo February 3, 2025 14:54

itsmeichigo requested changes Feb 4, 2025

View reviewed changes

pmusolino and others added 5 commits February 4, 2025 11:56

Update WooCommerce/Classes/Authentication/Application Password/Applic…

6467c96

…ationPasswordAuthorizationWebViewController.swift Co-authored-by: Huong Do <[email protected]>

Update WooCommerce/Classes/Authentication/Navigation Exceptions/Appli…

71d68ee

…cationPasswordDisabledViewModel.swift Co-authored-by: Huong Do <[email protected]>

fix: naming

ec50237

Add previousViewController parameter to PostSiteCredentialLoginChecke…

fc9335d

…rTests: update `PostSiteCredentialLoginChecker` instantiation in `PostSiteCredentialLoginCheckerTests.swift` to include `previousViewController: nil` across multiple test cases.

pmusolino added 3 commits February 5, 2025 13:08

Merge branch 'trunk' into issue/14004-better-application-password-dis…

3e5b2ad

…abled-note-take-2

feat: remove weak from `ApplicationPasswordAuthorizationWebViewContro…

daee4c0

…ller`

update: navigation logic in ApplicationPasswordDisabledViewModel: c…

dc68ebc

…hange navigationController method from `popToRootViewController` to `popViewController`

Merge branch 'trunk' into issue/14004-better-application-password-dis…

4ae12d2

…abled-note-take-2

Refactor error handling in `ApplicationPasswordAuthorizationWebViewCo…

9fafc53

…ntroller` and display errorUI instead of error alert

Merge branch 'trunk' into issue/14004-better-application-password-dis…

9e4663e

…abled-note-take-2

Merge branch 'trunk' into issue/14004-better-application-password-dis…

2c7f417

…abled-note-take-2

pmusolino requested a review from itsmeichigo February 7, 2025 11:51

wpmobilebot modified the milestones: 21.7, 21.8 Feb 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Login] Improve error messaging displaying a new error screen when application password is disabled #15031

[Login] Improve error messaging displaying a new error screen when application password is disabled #15031

pmusolino commented Jan 31, 2025

wpmobilebot commented Jan 31, 2025 •

edited

Loading

itsmeichigo left a comment

itsmeichigo Feb 3, 2025

pmusolino Feb 3, 2025

itsmeichigo Feb 3, 2025

pmusolino Feb 3, 2025

pmusolino commented Feb 3, 2025

itsmeichigo left a comment •

edited

Loading

itsmeichigo Feb 4, 2025

pmusolino Feb 4, 2025

itsmeichigo Feb 4, 2025

pmusolino Feb 4, 2025

itsmeichigo Feb 5, 2025

pmusolino Feb 5, 2025

pmusolino commented Feb 4, 2025

itsmeichigo commented Feb 5, 2025

pmusolino commented Feb 5, 2025

itsmeichigo commented Feb 6, 2025

pmusolino commented Feb 7, 2025

itsmeichigo commented Feb 7, 2025

pmusolino commented Feb 7, 2025

wpmobilebot commented Feb 7, 2025

		// show application password disabled error, and catch the last view controller that was showing before the Application Password flow.
		let previousViewController = navigationController.viewControllers.dropLast().last

[Login] Improve error messaging displaying a new error screen when application password is disabled #15031

Are you sure you want to change the base?

[Login] Improve error messaging displaying a new error screen when application password is disabled #15031

Conversation

pmusolino commented Jan 31, 2025

Description

Testing information

Screenshots

Reviewer (or Author, in the case of optional code reviews):

wpmobilebot commented Jan 31, 2025 • edited Loading

itsmeichigo 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

pmusolino commented Feb 3, 2025

itsmeichigo left a comment • 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

pmusolino commented Feb 4, 2025

itsmeichigo commented Feb 5, 2025

pmusolino commented Feb 5, 2025

itsmeichigo commented Feb 6, 2025

pmusolino commented Feb 7, 2025

itsmeichigo commented Feb 7, 2025

pmusolino commented Feb 7, 2025

wpmobilebot commented Feb 7, 2025

wpmobilebot commented Jan 31, 2025 •

edited

Loading

itsmeichigo left a comment •

edited

Loading