New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow non-Zulip.com installs to login via Google Auth #70

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@kunall17
Collaborator

kunall17 commented Jun 21, 2016

Fixed #64 ! :)
Used Open-ID Auth library to create the Auth System!

Requires two Client ID's in the settings.py here
'GOOGLE_CLIENT_ID'and 'GOOGLE_OAUTH2_CLIENT_ID'and ofcourse GoogleMobileOauth2Backend to be enabled in the Authentication Backends!

Demo:
google-sigin

@smarx

This comment has been minimized.

smarx commented Jun 21, 2016

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@@ -5,7 +5,7 @@ android {
buildToolsVersion "23.0.2"
defaultConfig {
applicationId "com.zulip.android"
minSdkVersion 13
minSdkVersion 16

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

This is a change that potentially has a pretty big impact on the Zulip install base. What's the reason for upping this to 16?

This comment has been minimized.

@kunall17

kunall17 Jun 22, 2016

Collaborator

When I added AppAuth dependency I got this error!

Error:Execution failed for task ':app:processDebugManifest'.
> Manifest merger failed : uses-sdk:minSdkVersion 13 cannot be smaller than version 16 declared in library [net.openid:appauth:0.2.0]
    Suggestion: use tools:overrideLibrary="net.openid.appauth" to force usage

AppAuth asks for 16, so I bumped it up. Didn't wanted to force usage.!

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

Cool. For now, let's assume it's ok :)

@@ -265,6 +182,93 @@ public void onTaskFailure(String result) {
}
}
private void setupSignIN(String clientId) {

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

The casing on this method name doesn't match convention. it should be setupSignIn.

@@ -265,6 +182,93 @@ public void onTaskFailure(String result) {
}
}
private void setupSignIN(String clientId) {
AuthorizationServiceConfiguration serviceConfiguration = new AuthorizationServiceConfiguration(
Uri.parse("https://accounts.google.com/o/oauth2/v2/auth") /* auth endpoint */,

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

These url strings should be kept in a constant rather than inline.

builder.setScopes("profile", "email");
AuthorizationRequest request = builder.build();
AuthorizationService authorizationService = new AuthorizationService(LoginActivity.this);
String action = "com.zulip.android.HANDLE_AUTHORIZATION_RESPONSE";

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

This is probably a good thing to keep as a constant, rather than inline as well.

public class AsyncFetchGoogleID extends ZulipAsyncPushTask {
/**
* Declares a new HumbugAsyncPushTask, passing the activity as context.

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

Humbug is a deprecated name for the Zulip project; this comment can be updated to read "Declares a new _Zulip_AsyncPushTask ..."

@@ -304,7 +308,7 @@ public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) {
mGoogleSignInButton.setVisibility(View.VISIBLE);
} else {
mServerEditText.setVisibility(View.VISIBLE);
mGoogleSignInButton.setVisibility(View.GONE);
// mGoogleSignInButton.setVisibility(View.GONE);

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

if this code is unused, it can be deleted.

public void onTaskComplete(String result, JSONObject jsonObject) {
try {
JSONObject jsonObject1 = new JSONObject(result);
setupSignIN(jsonObject1.getString("google_client_id"));

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

the signature for this method already returns the parsed JSONObject for the result. You can use that rather than creating a new one.

This comment has been minimized.

@kunall17

kunall17 Jun 23, 2016

Collaborator

This JSONObject is null

We need to fix this thing in maybe the okHttp PR.

JSONObject jsonObject1 = new JSONObject(result);
setupSignIN(jsonObject1.getString("google_client_id"));
} catch (JSONException e) {
e.printStackTrace();

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

Please use ZLog.e instead of printStackTrace

@Override
public void onTaskFailure(String result) {

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

Is there any action that we need to take on failure? Like hiding the login via G+ button or showing an error?

((ZulipApp) getApplication()).setEmail(object.getString("email"));
connectionProgressDialog.dismiss();
} catch (JSONException e) {
e.printStackTrace();

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

Please use ZLog.e instead of printStackTrace

public void onTaskFailure(String result) {
// Invalidate the token and try again, unless the user we
// are authenticating as is not registered or is disabled.
connectionProgressDialog.dismiss();

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

Is there any action that needs to be taken to invalidate the token?

}
}
private void handleAuthorizationResponse(Intent intent) {

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

This method is hard to read. If there's a way to clean it up or break out more of the logic into smaller sub-methods that'd be 👍

Uri.parse("https://www.googleapis.com/oauth2/v4/token") /* token endpoint */
);
Uri redirectUri = Uri.parse("com.zulip.android:/oauth2callback");

This comment has been minimized.

@niftynei

niftynei Jun 22, 2016

Contributor

I'm trying this out with http://zulip.tabbott.net and getting an error about a redirect_uri_mismatch. Is there a canonical callback endpoint that the zulip servers already use? If so, we should mimic that.

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jun 22, 2016

This looks really good overall @kunall17!! great job!!

A few things:
= If there's any errors in the server we're not doing a very good job of displaying them for the user. For example, if the server you're attempting to log into doesn't have a Google ID set up, we should remove the Google Login button in the UI.

= If you go to "Login with Google" and then click the X button to go back to the login screen, the spinner is still showing.

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jun 23, 2016

@niftynei Updated the Code! :)

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jun 23, 2016

Cool, thanks Kunal.

I'm having some trouble getting the login to complete correctly. I've got the Oauth Keys set up and can successfully complete the Oauth portion but when it navigates back to the app it seems to both log me out and successfully complete. As a result, I end up on a new LogInActivity (hitting the back button takes me to the first LogInActivity), but I can't successfully get to the messages view. I can see the app making all the right API calls to my dev server, but also making a call to log me out. Do you also have this behavior?

There's also these errors in the logs:


06-23 15:00:56.867 5090-5090/com.zulip.android.dev E/ActivityThread: Activity com.zulip.android.activities.LoginActivity has leaked ServiceConnection net.openid.appauth.BrowserHandler$1@f2300ab that was originally bound here
                                                                     android.app.ServiceConnectionLeaked: Activity com.zulip.android.activities.LoginActivity has leaked ServiceConnection net.openid.appauth.BrowserHandler$1@f2300ab that was originally bound here
                                                                         at android.app.LoadedApk$ServiceDispatcher.<init>(LoadedApk.java:1092)
                                                                         at android.app.LoadedApk.getServiceDispatcher(LoadedApk.java:986)
                                                                         at android.app.ContextImpl.bindServiceCommon(ContextImpl.java:1303)
                                                                         at android.app.ContextImpl.bindService(ContextImpl.java:1286)
                                                                         at android.content.ContextWrapper.bindService(ContextWrapper.java:604)
                                                                         at android.support.customtabs.CustomTabsClient.bindCustomTabsService(CustomTabsClient.java:60)
                                                                         at net.openid.appauth.BrowserHandler.bindCustomTabsService(BrowserHandler.java:86)
                                                                         at net.openid.appauth.BrowserHandler.<init>(BrowserHandler.java:61)
                                                                         at net.openid.appauth.AuthorizationService.<init>(AuthorizationService.java:98)
                                                                         at com.zulip.android.activities.LoginActivity.setupSignIn(LoginActivity.java:237)
                                                                         at com.zulip.android.activities.LoginActivity.access$000(LoginActivity.java:40)
                                                                         at com.zulip.android.activities.LoginActivity$1.onTaskComplete(LoginActivity.java:147)
                                                                         at com.zulip.android.networking.ZulipAsyncPushTask.onPostExecute(ZulipAsyncPushTask.java:145)
                                                                         at com.zulip.android.networking.ZulipAsyncPushTask.onPostExecute(ZulipAsyncPushTask.java:24)
                                                                         at android.os.AsyncTask.finish(AsyncTask.java:651)
                                                                         at android.os.AsyncTask.-wrap1(AsyncTask.java)
                                                                         at android.os.AsyncTask$InternalHandler.handleMessage(AsyncTask.java:668)
                                                                         at android.os.Handler.dispatchMessage(Handler.java:102)
                                                                         at android.os.Looper.loop(Looper.java:148)
                                                                         at android.app.ActivityThread.main(ActivityThread.java:5422)
                                                                         at java.lang.reflect.Method.invoke(Native Method)
                                                                         at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
                                                                         at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)

06-23 15:16:37.083 8584-8584/com.zulip.android.dev I/GOOGLE_SIGN: Handled Authorization Response {"scope":"profile email","lastAuthorizationResponse":{"request":{"configuration":{"authorizationEndpoint":"https:\/\/accounts.google.com\/o\/oauth2\/v2\/auth","tokenEndpoint":"https:\/\/www.googleapis.com\/oauth2\/v4\/token"},"clientId":"740118079718-nc6dcpssofjdg0i84r89gita43o9jbuc.apps.googleusercontent.com","responseType":"code","redirectUri":"com.zulip.android:\/oauth2callback","scope":"profile email","state":"EYil7D70UUUvg5fs3nlHpA","codeVerifier":"X3phVzGiDxoVxLG-AEVYEalH-q6sPF4yrxDdDqsUwJlGzE310UFlV85g2d9SW1Ahkk-MrDjEEiSp-sOVD407rA","codeVerifierChallenge":"a0R4MK66ycPpya2qh_2bKCYJ1FRtVG9clcanfUwYk0E","codeVerifierChallengeMethod":"S256","additionalParameters":{}},"state":"EYil7D70UUUvg5fs3nlHpA","code":"4\/aJBrq-ZPzF3d6TDSO4zn9svZnUWOFpY4zNcBso2phKE","additional_parameters":{}}} 
06-23 15:16:37.329 8584-8584/com.zulip.android.dev W/GOOGLE_SIGN: Token Exchange failed
                                                                  AuthorizationException: {"type":0,"code":3,"errorDescription":"Network error"}
                                                                      at net.openid.appauth.AuthorizationService$TokenRequestTask.doInBackground(AuthorizationService.java:244)
                                                                      at net.openid.appauth.AuthorizationService$TokenRequestTask.doInBackground(AuthorizationService.java:206)
                                                                      at android.os.AsyncTask$2.call(AsyncTask.java:295)
                                                                      at java.util.concurrent.FutureTask.run(FutureTask.java:237)
                                                                      at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:234)
                                                                      at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113)
                                                                      at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588)
                                                                      at java.lang.Thread.run(Thread.java:818)
                                                                   Caused by: java.io.FileNotFoundException: https://www.googleapis.com/oauth2/v4/token
                                                                      at com.android.okhttp.internal.huc.HttpURLConnectionImpl.getInputStream(HttpURLConnectionImpl.java:238)
                                                                      at com.android.okhttp.internal.huc.DelegatingHttpsURLConnection.getInputStream(DelegatingHttpsURLConnection.java:210)
                                                                      at com.android.okhttp.internal.huc.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java)
                                                                      at net.openid.appauth.AuthorizationService$TokenRequestTask.doInBackground(AuthorizationService.java:239)
                                                                      at net.openid.appauth.AuthorizationService$TokenRequestTask.doInBackground(AuthorizationService.java:206) 
                                                                      at android.os.AsyncTask$2.call(AsyncTask.java:295) 
                                                                      at java.util.concurrent.FutureTask.run(FutureTask.java:237) 
                                                                      at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:234) 
                                                                      at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113) 
                                                                      at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588) 
                                                                      at java.lang.Thread.run(Thread.java:818) 
@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jun 24, 2016

If I use my keys for the Web client I too receive the same error!
I have this setup
'GOOGLE_CLIENT_ID' as the Android client
'GOOGLE_OAUTH2_CLIENT_ID' as the web client in my server!

06-24 11:16:58.860 2810-2810/com.zulip.android.dev W/GOOGLE_SIGN: Token Exchange failed
                                                                  AuthorizationException: {"type":0,"code":3,"errorDescription":"Network error"}
                                                                      at net.openid.appauth.AuthorizationService$TokenRequestTask.doInBackground(AuthorizationService.java:244)
                                                                      at net.openid.appauth.AuthorizationService$TokenRequestTask.doInBackground(AuthorizationService.java:206)
                                                                      at android.os.AsyncTask$2.call(AsyncTask.java:292)
                                                                      at java.util.concurrent.FutureTask.run(FutureTask.java:237)
                                                                      at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:231)
                                                                      at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
                                                                      at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
                                                                      at java.lang.Thread.run(Thread.java:818)
                                                                   Caused by: java.io.FileNotFoundException: https://www.googleapis.com/oauth2/v4/token
                                                                      at com.android.okhttp.internal.http.HttpURLConnectionImpl.getInputStream(HttpURLConnectionImpl.java:206)
                                                                      at com.android.okhttp.internal.http.DelegatingHttpsURLConnection.getInputStream(DelegatingHttpsURLConnection.java:210)
                                                                      at com.android.okhttp.internal.http.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:25)
                                                                      at net.openid.appauth.AuthorizationService$TokenRequestTask.doInBackground(AuthorizationService.java:239)
                                                                      at net.openid.appauth.AuthorizationService$TokenRequestTask.doInBackground(AuthorizationService.java:206) 
                                                                      at android.os.AsyncTask$2.call(AsyncTask.java:292) 
                                                                      at java.util.concurrent.FutureTask.run(FutureTask.java:237) 
                                                                      at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:231) 
                                                                      at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112) 
                                                                      at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587) 
                                                                      at java.lang.Thread.run(Thread.java:818) 

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jun 24, 2016

@niftynei for the If you go to "Login with Google" and then click the X button to go back to the login screen, the spinner is still showing.

To set a CustomTabsCallback (callback which gets the events in chrome custom tabs) is currently not available in the 0.2.0 version of openId AppAuth library, I have made changes in the library for this callback and posted a PR in their repository but I'm not sure when this will be merged and a new version of the library will be released , they had talks about new version coming up soon last month.
Currently I have dismissed the dialog when the Google client ID is fetched from the server as after that the custom chrome tabs show up for, which is not a great solution.

Will it be right to include their library as a module and compile with the fix in our gradle?

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 10, 2016

@niftynei updated the branch! :)

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 19, 2016

@kunall17 were you able to successfully use this to log in to a server other than your dev server? I believe http://zulip.tabbott.net is set up to do this now.

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 19, 2016

@kunall17 I haven't been able to get this branch to build. I'm getting org.apache.http.client does not exist errors on AsyncGoogleIDFetch.

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 19, 2016

Damn, sorry the silly rebase conflicts.
Btw we should really think to implement a CI to do these things for us?

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 19, 2016

Yeah, that's a good idea. @timabbott sent me one a few weeks ago, I'll look into setting it up.

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 19, 2016

@kunall17 I can't for the life of me get this working. I'm sending you an email to get some help debugging my setup. :(

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 21, 2016

@kunall17 if you could add the docs for how to set up a web server to use this that you're writing up to this PR, I'll go ahead and get it merged in. We can retro-actively fix any other errors that come up.

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 21, 2016

@niftynei I was just waiting for zulip.tabbott.net to get this thing working then I would go edit the docs!

@niftynei

This comment has been minimized.

Contributor

niftynei commented Sep 9, 2016

this has been fixed by using the same GOOGLE_CLIENT_ID for all installs.

@niftynei niftynei closed this Sep 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment