[REST API] Log user out if user is unauthorized to generate application password

[hichamboushaba]

Closes: #8221

Description

This PR simply monitors the application password generation failures, and log the user out when a failure due to a 401 error happens.

Testing instructions

1. Make sure you are assigned the Treatment variant of the REST API experiment (either by hardcoding it or by setting a test device)
2. Open the app, then sign in using site credentials.
3. Open your site, then change the user password, and revoke the application password.
4. Go back to the app, then do tasks to trigger API requests.
5. Confirm the app logs you out.

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

[wpmobilebot]

You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code:

[codecov-commenter]

Codecov Report

Base: 43.44% // Head: 43.59% // Increases project coverage by +0.15% 🎉

Coverage data is based on head (f96ebbc) compared to base (fa5fb4f).
Patch coverage: 59.14% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk    #8222      +/-   ##
============================================
+ Coverage     43.44%   43.59%   +0.15%     
- Complexity     3533     3584      +51     
============================================
  Files           713      720       +7     
  Lines         37954    38295     +341     
  Branches       5014     5051      +37     
============================================
+ Hits          16489    16696     +207     
- Misses        19993    20103     +110     
- Partials       1472     1496      +24     

Impacted Files	Coverage Δ
...n/kotlin/com/woocommerce/android/AppInitializer.kt	0.00% <0.00%> (ø)
.../kotlin/com/woocommerce/android/AppPrefsWrapper.kt	0.00% <0.00%> (ø)
.../woocommerce/android/analytics/AnalyticsTracker.kt	20.76% <ø> (ø)
...commerce/android/ui/login/LoginAnalyticsTracker.kt	0.00% <0.00%> (ø)
...oocommerce/android/ui/login/WPApiSiteRepository.kt	0.00% <0.00%> (ø)
...android/ui/payments/feedback/ipp/FeedbackBanner.kt	0.00% <0.00%> (ø)
...ents/feedback/ipp/MarkFeedbackBannerAsDismissed.kt	0.00% <0.00%> (ø)
...edback/ipp/MarkFeedbackBannerAsDismissedForever.kt	0.00% <0.00%> (ø)
...s/feedback/ipp/MarkIPPFeedbackSurveyAsCompleted.kt	0.00% <0.00%> (ø)
...plicationpasswords/ApplicationPasswordsNotifier.kt	2.43% <2.85%> (+2.43%)	⬆️
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[JorgeMucientes]

Hey @hichamboushaba Nicely done!
Approving the PR but I have a question. Is the log-out expected to happen after killing the app or should it happen even if the app is opened from the background (after the password has been revoked) ? During my tests I was only able to get logged out after killing and restarting the app.

[hafizrahman]

Question: Should we inform users about this, perhaps at least show a Toast? They might be in the middle of a pretty big task (e.g. updating a product) and might want to know why they're suddenly signed out.

[hichamboushaba]

Question: Should we inform users about this, perhaps at least show a Toast? They might be in the middle of a pretty big task (e.g. updating a product) and might want to know why they're suddenly signed out.

Good point @hafizrahman, but if it's OK with you, let's leave it for another PR, the error might be triggered multiple times if multiple calls were made, but we need to make sure the Toast is shown only once, and I don't want to delay the PR because of it.
Also, the error happens only if the user intentionally "changed the password" and "revoked the application password", so I don't think it could happen while they are doing something important in the app.

Approving the PR but I have a question. Is the log-out expected to happen after killing the app or should it happen even if the app is opened from the background (after the password has been revoked) ? During my tests I was only able to get logged out after killing and restarting the app.

@JorgeMucientes the error should be triggered by API calls, so if you try to refresh orders or products, it should be triggered without having to restart the app, can you check again? 🙏

[JorgeMucientes]

Hey @hichamboushaba 👋🏼
I tested it again following the same steps I did in my previous tests and still I had to kill the app and restart it to get the expected behaviour.

In this first scenario I log in using site credentials and revoke the app password as well as I change the user password:

revokeAppPasswordAndResetPassword.mov

In this other scenario I only revoke app password. Everything seem to be working fine, the app is able to recover and generate another App password. But then notice at the end of the video I revoke again the app password without changing the user password and I'm instantly logged out from the app:

revokeAppPasswordOnlyTrimmed.mov

And finally, I also experienced this rare behaviour right when I'm logged in, requests start failing and I'm automatically logged out, even when password was correctly generated:

loggedOutAfterLogin.mov

I'm the only one experiencing this weird behaviors? 😅

[hichamboushaba]

Thanks so much @JorgeMucientes for the videos, they are very helpful.

In this first scenario I log in using site credentials and revoke the app password as well as I change the user password:
I see what's difference between our tests, when I tested, I was already logged in, so can you try this scenario please:

1. Open the app then sign in.
2. Force close the app.
3. Open the app and confirm everything still works fine.
4. Now change the password and revoke the application password.
5. Try any API call.
6. The app should log you out.

The difference here is that we don't have a cached nonce, so we get the expected response 401, while in the other case, since the error happens just after the login, we get an an unexpected error 403 with the message rest_cookie_invalid_nonce, this is an unlikely scenario for the users (the users won't revoke the password just right after login), but we will fix it, just in a separate PR as it requires a FluxC change.

In this other scenario I only revoke app password. Everything seem to be working fine, the app is able to recover and generate another App password. But then notice at the end of the video I revoke again the app password without changing the user password and I'm instantly logged out from the app:

And finally, I also experienced this rare behaviour right when I'm logged in, requests start failing and I'm automatically logged out, even when password was correctly generated:

I can't reproduce these scenarios, in the videos I see some 429 Too Many Requests errors, which means the site rate limited the app.
Anyway, in the last commit, I updated the logout logic to use the correct logout function, which would cleanup the data (including removing the site from the DB), hopefully, this improves things, if you can re-test please, otherwise let's merge the PR and re-work on the issues if they are still happening on a separate PR.

[hichamboushaba]

This new argument was added in the commit 4eeb55a, but it was added only in the main_nav_graph.
Given that this destination is declared in two navgraphs, we need to make sure the arguments are kept in sync, otherwise the build fails randomly (like this build), this is a known bug, and we faced it before p1646845001099659-slack-C6H8C3G23

[hichamboushaba]

cc @samiuelson

[samiuelson]

Ouch, sorry for letting the bug in. Thank you for fixing this @hichamboushaba!

[peril-woocommerce]

	Warnings
⚠️	PR is not assigned to a milestone.

Generated by 🚫 dangerJS

[JorgeMucientes]

Thanks for looking into the possible issues shown in the videos @hichamboushaba.
Thanks for clarifying the potential issues regarding the firs scenario (video). Since that one is the most relevant case of the 3 and is a corner case I believe is safe to merge.