Back-end changes for Google XOAUTH2 #1747

philipwhiuk · 2016-10-24T11:31:31Z

Splitting out from #1295

Valodim

Lookin' good! Probably the most important PR we have currently, thanks for working on this 👍

Valodim · 2016-10-26T20:11:42Z

k9mail-library/src/main/java/com/fsck/k9/mail/Authentication.java

@@ -82,4 +84,10 @@ public static String computeCramMd5(String username, String password, String b64
            throw new MessagingException("Something went wrong during CRAM-MD5 computation", e);
        }
    }
+
+    public static String computeXoauth(String username, String authToken) throws UnsupportedEncodingException {
+        return new String(


this line is packed with logic, a couple of intermediate steps probably couldn't hurt

Valodim · 2016-10-26T20:12:37Z

k9mail-library/src/main/java/com/fsck/k9/mail/oauth/XOAuth2ChallengeParser.java

+    public static boolean shouldRetry(String response, String host) {
+        String decodedResponse = Base64.decode(response);
+
+        Log.v(LOG_TAG, "Challenge response: "+ decodedResponse);


seems like a candidate for if (K9.DEBUG)

Valodim · 2016-10-26T20:13:02Z

k9mail-library/src/main/java/com/fsck/k9/mail/oauth/XOAuth2ChallengeParser.java

+
+        try {
+            JSONObject json = new JSONObject(decodedResponse);
+            if(!json.getString("status").equals(BAD_RESPONSE)) {


code style :)

also, CONSTANT.equals(variable) avoids NPEs

Valodim · 2016-10-26T20:14:24Z

k9mail-library/src/main/java/com/fsck/k9/mail/oauth/XOAuth2ChallengeParser.java

+                return false;
+            }
+        } catch (JSONException jsonException) {
+            Log.i(LOG_TAG, "Error decoding JSON response from:"


shouldn't this be Log.e?

Valodim · 2016-10-26T20:20:05Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapConnection.java

@@ -322,6 +329,14 @@ private void startTLS() throws IOException, MessagingException, GeneralSecurityE
    @SuppressWarnings("EnumSwitchStatementWhichMissesCases")
    private void authenticate() throws MessagingException, IOException {
        switch (settings.getAuthType()) {
+            case XOAUTH2:
+                if (hasCapability(Capabilities.AUTH_XOAUTH2) && hasCapability(Capabilities.SASL_IR)
+                        && oauthTokenProvider != null) {


Is there a case where oauthTokenProvider is null here and it's not a bug? Either way it should probably be handled in an exception of its own, since the error message below is wrong in this case.

Valodim · 2016-10-26T20:21:51Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapConnection.java

+        try {
+            attemptXOAuth2();
+        } catch (NegativeImapResponseException e) {
+            //We couldn't login with the token so invalidate it


pointless comment :)

Valodim · 2016-10-26T20:23:02Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapConnection.java

+                    response.getString(0), settings.getHost());
+        }
+
+        if(response.isContinuationRequested()) {


could you try to be more careful about trivial style errors like this? android studio can do this for you (and us)

If it was me I'd have a commit that fixed all the errors and made the build fail if new ones were added but nobody's a fan of that :(

I'll fix it.

Valodim · 2016-10-26T20:23:54Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapConnection.java

@@ -635,6 +719,31 @@ protected String getLogId() {
        return responseParser.readStatusResponse(tag, commandToLog, getLogId(), untaggedHandler);
    }

+    public String sendSaslIrCommand(String command, String initialClientResponse, boolean sensitive)
+    throws IOException, MessagingException {


indentation

Valodim · 2016-10-26T20:25:32Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapResponseParser.java

@@ -109,7 +109,7 @@ private void readTaggedResponse(ImapResponseCallback callback) throws IOExceptio
                continue;
            }

-            if (untaggedHandler != null) {
+            if (response.getTag() == null && untaggedHandler != null) {


Just pointing out this might be a significant change in behavior, please double-check this doesn't break anything :)

I think it only didn't fail a test because the test was badly written. I wrote some better tests for it and I think it should have failed. To me it looked like faulty logic and I browsed the spec a bit.

I'll keep using it on my device though.

Valodim · 2016-10-26T22:33:53Z

k9mail-library/src/main/java/com/fsck/k9/mail/transport/SmtpTransport.java

+    }
+
+    private CommandResponse executeSimpleCommandWithResponse(String command, boolean sensitive) throws IOException, MessagingException {
+        List<String> results = new ArrayList<String>();


diamond operator~

jberkel · 2016-11-25T00:17:53Z

k9mail-library/src/main/java/com/fsck/k9/mail/oauth/OAuth2TokenProvider.java

+
+import java.util.List;
+
+public interface OAuth2TokenProvider {


where is this interface actually implemented? don't see it in this PR.

It's not. This is just the back-end changes. That's an interface for the front-end to implement.

Ah OK, makes sense, was confused because OAuth2TokenProvider sounds like back-end functionality. Does only authorizeAPI() need user interaction?

That's the expectation, yes.

An (imperfect) implementation that supports GMail only is here: https://github.com/philipwhiuk/k-9/commit/7bd0f52c6d24ba1696620c4109c30d1f06aa770f#diff-4e75c10b7bac153fec6888b448a738ea

HLFH · 2016-12-12T13:09:17Z

Thanks for that great work!

I hope we'll get a release soon to try this great feature.

sivaraam · 2018-06-04T10:09:29Z

I came here after some time of searching for open issues about Gmail OAuth logins. It's been around 1.5 years and as far as I could see I've not found OAuth support for logging into Gmail in the app. What's the current progress of this?

Robert-Ernst · 2019-01-31T15:31:15Z

@sivaraam Don't worry. Nothing has changed since your last message until now.

wiktor-k · 2019-01-31T20:00:18Z

Actually there is a PR #3804 that enables Gmail OAuth login.

philipwhiuk added 2 commits October 24, 2016 12:27

Back-end changes for Google XOAUTH2

7774ebc

Minor changes to front-end and interface to handle no implementation

25cba11

philipwhiuk mentioned this pull request Oct 25, 2016

Hotmail IMAP XOAUTH2 #1753

Closed

Valodim suggested changes Oct 26, 2016

View reviewed changes

Clean-up following code review thanks to Valodim

79c1f92

philipwhiuk added the type: security Issues related to security vulnerabilities label Nov 16, 2016

jberkel reviewed Nov 25, 2016

View reviewed changes

Valodim approved these changes Dec 7, 2016

View reviewed changes

cketti added 2 commits December 12, 2016 02:48

Code style fixes

74017c9

Don't pass OAuth2TokenProvider to RemoteStore and Transport

f9ed304

cketti force-pushed the xoauth2Backend branch from 02e9837 to f9ed304 Compare December 12, 2016 01:50

cketti merged commit 15ca924 into master Dec 12, 2016

cketti deleted the xoauth2Backend branch December 12, 2016 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Back-end changes for Google XOAUTH2 #1747

Back-end changes for Google XOAUTH2 #1747

philipwhiuk commented Oct 24, 2016

Valodim left a comment

Valodim Oct 26, 2016

Valodim Oct 26, 2016

Valodim Oct 26, 2016

Valodim Oct 26, 2016

Valodim Oct 26, 2016

Valodim Oct 26, 2016

Valodim Oct 26, 2016

philipwhiuk Oct 27, 2016 •

edited

Valodim Oct 26, 2016

Valodim Oct 26, 2016

philipwhiuk Oct 27, 2016

Valodim Oct 26, 2016

jberkel Nov 25, 2016

philipwhiuk Nov 25, 2016

jberkel Nov 25, 2016

philipwhiuk Nov 25, 2016

HLFH commented Dec 12, 2016

sivaraam commented Jun 4, 2018

Robert-Ernst commented Jan 31, 2019

wiktor-k commented Jan 31, 2019


		import java.util.List;

		public interface OAuth2TokenProvider {

Back-end changes for Google XOAUTH2 #1747

Back-end changes for Google XOAUTH2 #1747

Conversation

philipwhiuk commented Oct 24, 2016

Valodim 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipwhiuk Oct 27, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HLFH commented Dec 12, 2016

sivaraam commented Jun 4, 2018

Robert-Ernst commented Jan 31, 2019

wiktor-k commented Jan 31, 2019

philipwhiuk Oct 27, 2016 •

edited