Improve self-hosted sites login by daniloercoli · Pull Request #3819 · wordpress-mobile/WordPress-Android

daniloercoli · 2016-03-03T17:13:54Z

Fix #3805 by providing an enhanced version of the XML-RPC discovery procedure and some other nice things ;)

This PR also checks that the found endpoint does have the basic XML-RPC methods required by the app. (We may want to extended the list if necessary).
The Bad Behavior plugins matching is ready, but moved in the future folder. If you feel confident in keeping the BB plugins list updated (wordpress-mobile/app-blocking-plugins) feel free to put it back in production.
The Bad Behavior matching seems to be solid, since the "main" logic behind it was taken from wpscan, but I didn't test it too much. In the end it is just a series of tests upon HTTP response codes returned by the external web server.
Also added a new Analytics event LOGIN_INSERTED_INVALID_URL (login_inserted_invalid_url), that has 1 property attached to it user_inserted_url, that keep tracks of how many invalid URLs we receive in the login screen. I've not added it to Mixpanel Should I?

cc @oguzkocer @hypest @SergioEstevao @bummytime @astralbodies

…re available on the server.

…nor issues.

…ss-Android into feature/3805-improve-self-hosted-login-internals

Fixed a bunch of other issues along the way.

…eric one.

…ss-Android into feature/3805-improve-self-hosted-login-internals

…e server replies with an empty document.

…log.

…droid platform classes and methods.

… used now.

hypest · 2016-03-04T08:54:22Z

I'm planning to extend the work done here by extracting the cross-platform flows into testable chunks.

oguzkocer · 2016-03-04T15:45:42Z

WordPress/src/main/java/org/wordpress/android/ui/accounts/helpers/FetchBlogListWPOrg.java

-    }
-
    private boolean isHTTPAuthErrorMessage(Exception e) {
        if (e != null && e.getMessage() != null && e.getMessage().contains("401")) {


Maybe we can just return this since we are not doing anything else in the if statement anymore.

Right. I've removed the unnecessary code.

oguzkocer · 2016-03-07T12:07:02Z

WordPress/src/future/res/PluginsCheckerWPOrg.java

+        String baseURL = getBaseURL(mOriginalURL);
+
+        if (!baseURL.contains("/plugins")) {
+            baseURL = baseURL + "/wp-content/plugins/";


This might be a stupid question as I don't know how this class is going to be used, but is there any way the url contains /wp-content but not /plugins?

Please ignore the above question, I found my answer in getBaseUrl, sorry about that!

oguzkocer · 2016-03-07T12:14:21Z

WordPress/src/future/res/PluginsCheckerWPOrg.java

+        }
+
+        int respCode = openConnection(baseURL);
+        if (respCode != HttpURLConnection.HTTP_OK && respCode != 401 && respCode != 403) {


I am a bit confused here, what do we do if the error code is 401 or 403 for example? These cases don't seem to be handled later.

OK, I have an idea. Is this just checking if we can access the server even if it's an unauthorized connection? If so, then this makes sense. However, wouldn't that cause the next call we are going to make to check if a plugin exists return error as well? I really don't know much about how self-hosted installations work, so I might be way off in my assumptions.

We need to check wether the http://domain/wp-content/plugins/ address is available on internet or not. If the HTTP response code in [200, 401, 403] means that the folder is available on the server. If the response is != from one of the codes above, we can safely assume that the folder is not there.
Note that the routine is just an heuristic, and we can't be 100% sure the folder is there.

note2: Assuming that the folder is NOT there is not a problem, since the plugins check is just skipped. This minimize false positive.

Thanks for the clarification. It's pretty convoluted.

oguzkocer · 2016-03-07T13:17:20Z

@daniloercoli I've completed my review and left a few comments/questions. This is a complicated PR for me to take alone, so getting another set of eyes (more experienced ones) would be good.

Are there any testing instructions I can follow?

…tring

daniloercoli · 2016-03-07T14:37:58Z

@oguzkocer I fixed the issues you've brought up during review.
We've a list of self-hosted sites, might make sense to try the new login on them.
I've tested the patch locally, by messing up with a local installation of WP.

Hint for reviewers: The best way to review all the login flow is to start looking at FetchBlogListWPOrg.getSelfHostedXmlrpcUrl and follow the flow from there. Looking at the diff could be hard to follow the new logic.

…ss-Android into feature/3805-improve-self-hosted-login-internals # Conflicts: # WordPress/src/main/java/org/xmlrpc/android/ApiHelper.java

@hypest

…after a SSLHanshakeException on self hosted sites. Props @hypest

…ted a valid URL on the UI, but WordPress is missconfigured and returns an invalid URL in the apiLink field in the RSD document.

daniloercoli · 2016-03-11T11:30:53Z

Closing this PR since #3853 includes the changes added here, and adds Unit Tests.

daniloercoli added 22 commits February 29, 2016 18:05

Try the user inserted URL first, and check that all XML-RPC methods a…

e70a334

…re available on the server.

Remove 'wp-login.php' from input URL

b7f5958

Remove junk before the xml preamble in the RSD parsing method.

db46961

Remove "wp-login.php" if available in the input URL, and fix other mi…

57ea8e6

…nor issues.

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

d4a1b91

…ss-Android into feature/3805-improve-self-hosted-login-internals

Introduce "smart" algorithms that guess the URL of the site.

6ffbacf

Fixed a bunch of other issues along the way.

Always use lowercase URL. This commit needs to be double checked.

6f47e4c

Refactoring the code that check the validity of the XML-RPC Endpoint

c87afdf

Check for BB plugins on the server.

9356320

Added few more comments, and cleaned the code a bit.

c267865

Refactoring.

fdab062

Reverting 6f47e4c - don't force the user inserted URL to lowercase.

9e7fa5e

Better logging

4c00304

Catch Timeout error and show a detailed error mesage instead of a gen…

2f297d4

…eric one.

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

a5a6198

…ss-Android into feature/3805-improve-self-hosted-login-internals

Move the document with BB plugins to xmlrpc.eritreo.it.

4760505

Do not return xml-rpc endpoint found but have missing methods when th…

07f2341

…e server replies with an empty document.

Correctly catch redirect errors on login, and remove the Crashlytics …

ef7a750

…log.

Remove any external dependencies from the PluginsChecker. Just use An…

60e4d23

…droid platform classes and methods.

Download the BB plugins list from the new shiny repo on Github

10583c3

Move the plugins checker to the "future" folder. It's not going to be…

d122b49

… used now.

Bump Analytics when the user inserts an invalid URL

a5afb6a

daniloercoli added [Type] Enhancement Core Analytics labels Mar 3, 2016

Remove unused imports.

fcf063d

oguzkocer reviewed Mar 4, 2016
View reviewed changes

Removed unnecessary if statement.

9f87768

oguzkocer reviewed Mar 7, 2016
View reviewed changes

daniloercoli added 2 commits March 7, 2016 15:27

Correctly use break instead of continue to exit a loop immediately.

1f3e7b1

Make 'appendXMLRPCPath' more readable by using charAt instead of subs…

1db3fba

…tring

daniloercoli added 3 commits March 10, 2016 13:01

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

ff22486

…ss-Android into feature/3805-improve-self-hosted-login-internals # Conflicts: # WordPress/src/main/java/org/xmlrpc/android/ApiHelper.java

Fix an issue where the "erroneous SSL cert" flag was not set to true …

e0ac745

…after a SSLHanshakeException on self hosted sites. Props @hypest

Do not track the "Invalid inserted URL" event when the user has inser…

6826b58

…ted a valid URL on the UI, but WordPress is missconfigured and returns an invalid URL in the apiLink field in the RSD document.

hypest mentioned this pull request Mar 11, 2016

New .ORG login "internals" & Health-Check MVP #3853

Merged

daniloercoli closed this Mar 11, 2016

maxme deleted the feature/3805-improve-self-hosted-login-internals branch March 23, 2016 14:16

Conversation

daniloercoli commented Mar 3, 2016

Uh oh!

hypest commented Mar 4, 2016

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!

oguzkocer commented Mar 7, 2016

Uh oh!

daniloercoli commented Mar 7, 2016

Uh oh!

daniloercoli commented Mar 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants