Run XML-RPC discovery during SiteAddress login flow in WCAndroid

malinajirka commented 2 years ago

Fixes https://github.com/woocommerce/woocommerce-android/issues/3767

This PR fixes WCAndroid app which is showing "Jetpack not connected" screen even when Jetpack is connected. This is caused by a bug on the api - isJetpackConnected returns false when xml-rpc is disabled.

This PR fixes this issue by always running "xml-rpc discovery" in the SiteAddress login flow. This will have some performance impact, since we'll need to run an extra request, but should significantly help with login issues as the error messages contain specific instructions.

Some more details on p91TBi-5Y9-p2

Heads-up: I'm not too familiar with the login flows and I might be missing something, so please review this PR carefully. Thanks ;).

To test:

the app will now run "xml-rpc discovery" and show all these errors
the ssl error is not handled in the woo app and it's out of the scope of this PR - I'd suggest logging an exception as the first step -> I still consider this PR as an enhancement even though this scenario is still not handled

Test site address login in the woo app - valid and invalid cases (anything you can think of).

I tested

Jetpack not connected(https://easy-rhino.jurassic.ninja) - jetpack not connected error shown
Jetpack connected but xmlrpc disabled using .htaaccess method (https://deep-louse.jurassic.ninja) - xmlrpc error shown
Jetpack not installed (https://supposed-nurse.jurassic.ninja) - jetpack not connected error shown
Working site - the user gets redirected to email login flow
Blocked XML-RPC (405 error) - (I used site from FluxC's testing properties) - xmlrpc error shown
Missing XML-RPC methods - (I used site from FluxC's testing properties) - xmlrpc error shown
Forbidden XML-RPC (403 error) - (I used site from FluxC's testing properties) - xmlrpc error shown
Http auth enabled(https://assistant-magpie.jurassic.ninja)⚠️ This scenario still doesn't work -> the app shows http auth dialog and accepts only valid credentials -> but /v1.1/connect/site-info/ returns "jetpack not connected" even though jetpack is connected (Ticket here)
```
exists: true
hasJetpack: true
isJetpackActive: false
isJetpackConnected: false
isWordPress: true
isWordPressDotCom: false
jetpackVersion: "10.1"
skipRemoteInstall: true
urlAfterRedirects: https://assistant-magpie.jurassic.ninja
```

cc @designsimply I'm not too familiar with the login flow and this change feels a bit risky. Could you perhaps find someone who could thoroughly test this to limit the chance we'll need to work on time-critical beta fixes? Thank you!

P.S. I plan to log the remaining issues

http auth enabled
self-signed ssl

when this PR is merged. In case we'd find some more errors or someone would have an idea for an easy fix.

anitaa1990 commented 2 years ago

@malinajirka, I think this could use a design review from Garance and Adam. Initially, we had screens like this where the error message was displayed before the site url edittext. But later changed that to be on a separate screen with specific actions for the user to do in order to fix that issue. This improved the login flow a bit and I wonder if we want to do something similar here 😅

malinajirka commented 2 years ago

@anitaa1990 I was thinking about it when I saw the "Jetpack not connected/Not a wordpress site" errors. However, I won't be able to improve the designs during this hackweek. Imho showing at least some msg to the users is still better than showing a completely unrelated message(eg. jetpack not connected instead of xmlrpc error). We could improve the designs during the next hackweek, wdyt?

anitaa1990 commented 2 years ago

@anitaa1990 I was thinking about it when I saw the "Jetpack not connected/Not a wordpress site" errors. However, I won't be able to improve the designs during this hackweek. Imho showing at least some msg to the users is still better than showing a completely unrelated message(eg. jetpack not connected instead of xmlrpc error). We could improve the designs during the next hackweek, wdyt?

That makes sense! Thanks @malinajirka!

malinajirka commented 2 years ago

I'll be afk next week - please feel free to fix issues found during the review but also feel free to leave it up to me when I get back. Thanks 🙇

shiki commented 2 years ago

I think this could use a design review from Garance and Adam.

FWIW, I asked them to review the iOS designs in https://github.com/woocommerce/woocommerce-ios/pull/3957#issuecomment-819829489 and Garance was fine with it. 🙂 On iOS, those designs are also embedded tightly in WPAuth, and changing them might affect WP so it's quite risky to change them.

designsimply commented 2 years ago

I am so sorry for the delay in response here! I fully expected to get to this earlier after being at a team meetup last week and I should have communicated better.

@malinajirka! This is brilliant! I tested with the sites from FluxC's testing properties and got all the expected errors in all the right places. I tested several normal login flows and didn't find any regressions. I also tried testing using the following plugins to disable XML-RPC and got a couple interesting results. I saw that https://github.com/woocommerce/woocommerce-android/pull/4799 did get merged and I don't think these are blocking but they might be worth looking into depending on whether we continue to see user feedback about getting blocked on sign-in with a perceived wrong error message. The only one that might be worth looking into sooner than later is the case where I got an error about the site not being a WordPress site when it is a WordPress site because that case does come up in app reviews from time to time.

https://wordpress.org/plugins/disable-xml-rpc/

I saw a fullscreen error about XML-RPC after activating this plugin and attempting to log in using the store address > site credentials flow. If I instead selected the store address > Wordpress.com login option, then I was able to log in no problem.

https://wordpress.org/plugins/disable-xml-rpc-api/

When I tried to use the store address > site credentials flow to log in while this plugin is enabled, the error incorrectly tells me that the site at my address is not a valid WordPress site.

https://wordpress.org/plugins/disable-xml-rpc-pingback/
https://wordpress.org/plugins/rename-xml-rpc/

I was able to log in normally using the store address > site credentials flow with those two. 👍

^{Tested with WCAndroid pr-4799-build-49185 via this generated APK on Pixel 3 Android 11 trying to log in with store credentials on site strong-huntsman.jurassic.ninja running WordPress 5.8.1, Jetpack 10.1, WooCommerce 5.7.1.}

malinajirka commented 2 years ago

Thanks so much for the thorough testing @designsimply! ;)

https://wordpress.org/plugins/disable-xml-rpc/ I saw a fullscreen error about XML-RPC after activating this plugin and attempting to log in using the store address > site credentials flow. If I instead selected the store address > Wordpress.com login option, then I was able to log in no problem.

This is imo expected - the plugin by default whitelists connection from Jetpack's IP address. In other words it disables xmlrpc for all connections expect of Jetpack connection. When you try to login using using self-hosted credentials with xmlrpc dissbled the app can't communicate with the site. However, if you login using .com credentials - jetpack is able to communicate with the site's xmlrpc since it's whitelisted.

https://wordpress.org/plugins/disable-xml-rpc-api/ When I tried to use the store address > site credentials flow to log in while this plugin is enabled, the error incorrectly tells me that the site at my address is not a valid WordPress site.

It seems the code which wrongly assumes the site is not a wordpress site has been there since 2016. AFAICT the app is looking for a RSD link to get xmlrpc url of the site - if that link is not available on the homepage it considers the site as a non-wordpress site. My guess is that the disable-xml-rpc plugin removes this link and the app stops considering it as a wordpress site. I've created a workaround in this PR which skips the xmlrpc check if jetpack responds with "connected and active". That should fix this issue.

wordpress-mobile / WordPress-Login-Flow-Android