Add support for GitHub IDP #1199

SUPERCILEX · 2018-03-22T05:26:17Z

Fixes #346

@samtstern Alright, I'm so pumped right now! I figured out how to get GitHub working and I proved the flexibility and scalability of the new architecture—die two birds with my one stone! ☠️😆

Anyway, ignore this PR for now since I've already got you saturated everywhere else.

SUPERCILEX · 2018-03-22T05:30:24Z

Oh, and I forgot to mention that the build is going to fail because of translation issues. @samtstern Could you possibly kick off that process for these strings: https://github.com/firebase/FirebaseUI-Android/pull/1199/files#diff-059a92f7313626f64a0470b20e08afa4?

Signed-off-by: Alex Saveau <[email protected]>

# Conflicts: # auth/src/main/java/com/firebase/ui/auth/data/remote/SignInKickstarter.java # auth/src/main/java/com/firebase/ui/auth/ui/HelperActivityBase.java # auth/src/main/java/com/firebase/ui/auth/util/ExtraConstants.java

Signed-off-by: Alex Saveau <[email protected]>

# Conflicts: # auth/src/main/java/com/firebase/ui/auth/viewmodel/RequestCodes.java

Signed-off-by: Alex Saveau <[email protected]>

# Conflicts: # library/quality/quality.gradle

# Conflicts: # auth/build.gradle # auth/src/main/res/values/styles.xml

# Conflicts: # auth/build.gradle # library/quality/quality.gradle

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2018-06-13T04:40:42Z

@samtstern Think we can get the ball rolling on this one?

samtstern · 2018-06-13T15:44:46Z

@SUPERCILEX sure, happy to get this one back on track.

samtstern

Just some first-pass comments.

samtstern · 2018-06-13T21:58:02Z

auth/src/main/java/com/firebase/ui/auth/data/remote/GitHubSignInHandler.java

+
+    private interface GitHubOAuth {
+        @POST(KEY_ACCESS_TOKEN)
+        Call<JsonObject> getAuthToken(@Header("Accept") String header,


Can we get stronger typing than JsonObject here?

WHAT!!!? How has no one ever told me about this before now??? For some reason, it never clicked that I could use something other than JsonObject. 🤦‍♂️ Heck, I even use Gson to deserialize objects outside of Retrofit! Well, time to go update my projects again. 😆 @samtstern Thanks a ton for jostling those brain cells around! ❤️

TL;DR: yes. 👍

Hah that's funny that you didn't know because in my mind that's all Retrofit is good for!

Haha, yeah, I realize that now. 😊

samtstern · 2018-06-13T23:04:16Z

auth/src/main/java/com/firebase/ui/auth/ui/provider/GitHubLoginActivity.java

+                    .enableUrlBarHiding()
+                    .setToolbarColor(ContextCompat.getColor(this, R.color.colorPrimary))
+                    .build()
+                    .launchUrl(this, (Uri) getIntent().getParcelableExtra(ExtraConstants.PARAMS));


We're casting the whole PARAMS to a Uri? Maybe it should come with a different ExtraConstants key?

samtstern · 2018-06-13T23:04:49Z

auth/src/main/java/com/firebase/ui/auth/ui/provider/GitHubLoginActivity.java

+        if (mShouldCloseCustomTab) { // User pressed back
+            finish(RESULT_CANCELED, null);
+        }
+        mShouldCloseCustomTab = true;


Can you explain a little why this is set to true on each onResume?

Yeah, I'll explain this and the recursive launch comment below. Basically, I've copied-ish all of this from Facebook since they actually did a really nice job with CCT. So here's what we're trying to do:

Launch CCT

If user presses back, close resources related to CCT

Same for success and failure, but send result data too

Given that CCT is going to redirect to our activity, we need a wrapper with special stuff like singleTop and ignored config changes so the stack doesn't nest itself. Now that we're guaranteed to have a single activity with a CCT layer on top, we can safely assume that onCreate is the only place to start CCT. So the current flow looks like this:

Launch CCT in onCreate

Receive redirects in onNewIntent

That creates a problem though: how do we close CCT? Android doesn't give you a nice way to close all activities on top of the stack AFAIK, so we're force to relaunch our wrapper activity with the CLEAR_TOP flag. That will recurse while killing CCT to bring us back to onNewIntent again where we check for the refresh action. At that point, we can finally finish with our result.

Now for the onResume stuff. Remember how we always have a CCT layer on top? That means onResume will only ever be called once... unless the user presses back. At that point, our wrapper activity gains focus for the second time and we can safely kill it knowing it was a back event.

Whew! 😌 I realize that's a painful explanation. I dunno, Facebook's code was even more complicated and took me forever to even distill down to this point. They had 2 activities they were juggling around—never understood why. Anyway, if you have any ideas to simplify this stuff, please send them over! 😄

That's all super cool! Can you just encode some of this in code comments? If you want you can literally copy what you just said to a class-level comment.

samtstern · 2018-06-13T23:05:42Z

auth/src/main/java/com/firebase/ui/auth/ui/provider/GitHubLoginActivity.java

+                    .putExtra(GitHubSignInHandler.KEY_GITHUB_CODE, code);
+        }
+
+        // Force a recursive launch to clear the Custom Tabs activity


What's going on here?

See #1199 (comment)

samtstern · 2018-06-13T23:07:10Z

auth/src/main/res/values/config.xml

-           tools:ignore="ResourceName">
+<resources xmlns:tools="http://schemas.android.com/tools" tools:ignore="ResourceName">
+    <!-- Get access to the google-services project id. -->
+    <item name="project_id" type="string"/>


What does adding this item tag do?

Oh boy, forgot about that. Time to remove my hacks and write some docs! 😉

SUPERCILEX · 2018-06-18T17:48:37Z

So you added your project URL to the app config.xml and the asset links have the correct SHA-256? Usually, the non redirect means the hash doesn't match up with how the app was signed.

FYI gonna wait to see how you get through this to address your feedback 👍.

SUPERCILEX · 2018-06-18T17:49:18Z

Oh, just saw your edit, gimme a sec

SUPERCILEX · 2018-06-18T17:52:15Z

@samtstern can you check the app module's merged manifest? Is the value being pulled in or is lint just mad?

samtstern · 2018-06-18T17:58:56Z

Here's what I see in the merged manifest:

$ cat app/build/intermediates/merged_manifests/debug/processDebugManifest/merged/AndroidManifest.xml | grep -B1 -A15 GitHub
        <activity
            android:name="com.firebase.ui.auth.ui.provider.GitHubLoginActivity"
            android:configChanges="keyboard|keyboardHidden|screenLayout|screenSize|orientation"
            android:label=""
            android:launchMode="singleTop"
            android:theme="@style/FirebaseUI.Transparent" >
            <intent-filter android:autoVerify="true" >
                <action android:name="android.intent.action.VIEW" />

                <category android:name="android.intent.category.DEFAULT" />
                <category android:name="android.intent.category.BROWSABLE" />

                <data
                    android:host="@string/firebase_web_host"
                    android:path="/__/auth/handler"
                    android:scheme="https" />
            </intent-filter>

Which looks fine to me? I don't think it should be a string literal at that point.

SUPERCILEX · 2018-06-18T18:00:33Z

Oh yeah, sorry, I guess you'll have to use the APK analyzer to see the finished product.

samtstern · 2018-06-18T18:02:54Z

@SUPERCILEX here's what I see in APK analyzer:

        <activity
            android:theme="@ref/0x7f0f00d4"
            android:label="@string/0x21"
            android:name="com.firebase.ui.auth.ui.provider.GitHubLoginActivity"
            android:launchMode="1"
            android:configChanges="0x5b0">

            <intent-filter
                android:autoVerify="true">

                <action
                    android:name="android.intent.action.VIEW" />

                <category
                    android:name="android.intent.category.DEFAULT" />

                <category
                    android:name="android.intent.category.BROWSABLE" />

                <data
                    android:scheme="https"
                    android:host="@ref/0x7f0e005f"
                    android:path="/__/auth/handler" />
            </intent-filter>
        </activity>

And for comparison, Facebook:

        <activity
            android:theme="@ref/0x7f0f01b6"
            android:label="@ref/0x7f0e001f"
            android:name="com.facebook.FacebookActivity"
            android:configChanges="0x5b0" />

        <activity
            android:name="com.facebook.CustomTabActivity"
            android:exported="true">

            <intent-filter>

                <action
                    android:name="android.intent.action.VIEW" />

                <category
                    android:name="android.intent.category.DEFAULT" />

                <category
                    android:name="android.intent.category.BROWSABLE" />

                <data
                    android:scheme="@ref/0x7f0e005a" />
            </intent-filter>
        </activity>

So this may be a red herring.

samtstern · 2018-06-18T18:04:57Z

If I hardcode the value in auth/AndroidManifest.xml to my firebaseapp domain I get a little farther, to this point. Which makes me think we need more info in the error messages:

2018-06-18 11:03:53.223 13023-13023/com.firebase.uidemo E/AuthUI: A sign-in error occurred.
    com.firebase.ui.auth.data.model.UserCancellationException: Unknown error
        at com.firebase.ui.auth.data.remote.GitHubSignInHandler.onActivityResult(GitHubSignInHandler.java:109)
        at com.firebase.ui.auth.ui.idp.AuthMethodPickerActivity.onActivityResult(AuthMethodPickerActivity.java:234)
        at android.app.Activity.dispatchActivityResult(Activity.java:7276)
        at android.app.ActivityThread.deliverResults(ActivityThread.java:4264)
        at android.app.ActivityThread.handleSendResult(ActivityThread.java:4312)
        at android.app.ActivityThread.-wrap19(Unknown Source:0)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1644)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loop(Looper.java:164)
        at android.app.ActivityThread.main(ActivityThread.java:6494)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807)
2018-06-18 11:03:53.225 13023-13023/com.firebase.uidemo E/AuthUI: A sign-in error occurred.
    com.firebase.ui.auth.data.model.UserCancellationException: Unknown error
        at com.firebase.ui.auth.data.remote.GitHubSignInHandler.onActivityResult(GitHubSignInHandler.java:109)
        at com.firebase.ui.auth.ui.idp.AuthMethodPickerActivity.onActivityResult(AuthMethodPickerActivity.java:234)
        at android.app.Activity.dispatchActivityResult(Activity.java:7276)
        at android.app.ActivityThread.deliverResults(ActivityThread.java:4264)
        at android.app.ActivityThread.handleSendResult(ActivityThread.java:4312)
        at android.app.ActivityThread.-wrap19(Unknown Source:0)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1644)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loop(Looper.java:164)
        at android.app.ActivityThread.main(ActivityThread.java:6494)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807)

Edit: Ignore this, I think this was a debugging artifact.

SUPERCILEX · 2018-06-18T18:08:13Z

So is the custom tab just closing itself? Or is it still stuck at the redirect URI? I'm pretty sure your Sha hash is wrong...

SUPERCILEX · 2018-06-18T18:09:36Z

Actually, if you want to test that the activity is at least registered correctly, click the open in chrome menu option when you're in the CCT. It should let you choose between chrome or FUI.

samtstern · 2018-06-18T18:12:41Z

Now the CCT just closes itself. One interesting log line that appears:

2018-06-18 11:12:06.867 17569-17632/com.firebase.uidemo E/GraphResponse: {HttpStatus: 404, errorCode: 803, subErrorCode: -1, errorType: OAuthException, errorMessage: (#803) Cannot query users by their username (CHANGE-ME)}

Edit: I think that's unrelated Facebook junk.

SUPERCILEX · 2018-06-18T18:17:23Z

Mmmm... So if you go to your GitHub settings and revoke the tokens then log in, it brings you straight back to FUI (not the redirect URI)?

samtstern · 2018-06-18T18:18:29Z

I think this is just a typo. I did .wellknown not .well-known ... sigh gonna clear a bunch of caches and try again. Sorry for the time waste!

SUPERCILEX · 2018-06-18T18:19:19Z

Nooooooooooooooo!!! Sam! 😂

samtstern · 2018-06-18T18:23:35Z

I got it to work! Well I had to do the "Three Dots > Open In App > (select demo app)" but then it not only signed in with Github but also linked to my existing email/password account and perfectly updated the profile picture. Very impressed.

So remaining action items:

I still think my documentation comments are worth fixing.
I am gonna wait and try and get it to work without having to manually redirect the CCT. I assume I just have a bad cache somewhere from all my fiddling.

Update: Tried this on a fresh device and (1) is not an issue at all.

SUPERCILEX · 2018-06-18T18:29:18Z

Woot! 👏🍾 Yeah, I still want to improve the docs with your feedback and add a "WTF!? It's not working" section 😂

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2018-06-19T03:21:46Z

@samtstern The last two commits include a bunch of docs improvements, what do you think?

samtstern · 2018-06-19T19:38:16Z

@SUPERCILEX those are great! Final thing: let's just tone down the Github section a little bit to make it less scary. Removing things like "It's Github's fault" and "shooting yourself in the foot". Google has a really good relationship with Github and I don't wanna be out here criticizing them, even though it's good-natured.

SUPERCILEX · 2018-06-19T19:51:03Z

Haha, yeah, it could be taken the wrong way. 👍

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2018-06-19T20:06:20Z

@SUPERCILEX good to go!

SUPERCILEX · 2018-06-19T20:07:21Z

Side note: I actually got into contact with GitHub yesterday, and they pointed me to the OAuth spec they use with the security reasons behind their decision (which I've linked to in the README)—really interesting stuff. In any case, I pointed out that the spec does allow configuring multiple redirect URIs so they were kind enough to put that on the backlog... Down the road, we might be able to simplify all this! 👏

TL;DR: I wasn't really being fair anyway now that I understand their reasoning. 😊

SUPERCILEX · 2018-06-19T20:08:22Z

Awesome sauce! And there aren't even any conflicts with #1358. 🙌

samtstern · 2018-06-19T20:10:31Z

@SUPERCILEX that's good to hear that you got in contact with them. Also congrats on the first big feature on top of the new architecture. Very impressive how it scaled.

SUPERCILEX · 2018-06-19T20:13:42Z

Thanks! Yeah, I'm super happy we didn't get stuck at that 95%. 😉

SUPERCILEX requested a review from samtstern as a code owner March 22, 2018 05:26

SUPERCILEX mentioned this pull request Mar 22, 2018

Add Github as an IDP #346

Closed

Add support for GitHub IDP

375800a

Signed-off-by: Alex Saveau <[email protected]>

samtstern added this to the 3.4.0 milestone Mar 26, 2018

SUPERCILEX added 6 commits March 26, 2018 12:09

Fix merge mistakes

caa8257

Signed-off-by: Alex Saveau <[email protected]>

Fix more merge mistakes

396c620

Signed-off-by: Alex Saveau <[email protected]>

Start cleanup and officialization now that the diff is reasonable

dc8222b

Signed-off-by: Alex Saveau <[email protected]>

Add documentation

160996e

Signed-off-by: Alex Saveau <[email protected]>

Merge remote-tracking branch 'upstream/master' into github

f0c5afe

# Conflicts: # auth/src/main/java/com/firebase/ui/auth/viewmodel/RequestCodes.java

SUPERCILEX changed the base branch from version-3.3.0-dev to master March 28, 2018 21:49

samtstern changed the base branch from master to version-3.3.1-dev March 29, 2018 15:29

Merge remote-tracking branch 'upstream/version-3.3.1-dev' into github

e75ef82

samtstern changed the base branch from version-3.3.1-dev to version-3.4.0-dev April 13, 2018 19:48

SUPERCILEX added 2 commits April 20, 2018 21:07

Merge remote-tracking branch 'upstream/version-3.4.0-dev' into github

d90596f

Yes! Our proguard build caught an error!

4600290

Signed-off-by: Alex Saveau <[email protected]>

samtstern changed the base branch from version-3.4.0-dev to version-4.0.0-dev May 4, 2018 17:23

Merge remote-tracking branch 'upstream/version-4.0.0-dev' into github

282f166

# Conflicts: # library/quality/quality.gradle

samtstern modified the milestones: 4.0.0, 4.1.0 May 15, 2018

SUPERCILEX changed the base branch from version-4.0.0-dev to version-4.1.0-dev May 29, 2018 00:00

SUPERCILEX added 4 commits May 28, 2018 19:19

Merge remote-tracking branch 'upstream/master' into github

c7ba0e0

# Conflicts: # auth/build.gradle # auth/src/main/res/values/styles.xml

Merge branch 'version-4.1.0-dev' into github

c5278c6

# Conflicts: # auth/build.gradle # library/quality/quality.gradle

Fix merge mistakes

f082f09

Signed-off-by: Alex Saveau <[email protected]>

Fix merge mistakes

178589d

Signed-off-by: Alex Saveau <[email protected]>

samtstern reviewed Jun 13, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/version-4.1.0-dev' into github

cd27c48

SUPERCILEX added 2 commits June 18, 2018 20:11

Significant docs improvements

36cc5ec

Signed-off-by: Alex Saveau <[email protected]>

Add an "I'm stuck" section

c1412a2

Signed-off-by: Alex Saveau <[email protected]>

Remove possibly offensive language

ee30fe2

Signed-off-by: Alex Saveau <[email protected]>

samtstern merged commit 45c12b1 into firebase:version-4.1.0-dev Jun 19, 2018

SUPERCILEX deleted the github branch June 19, 2018 20:07

samtstern mentioned this pull request Jun 19, 2018

Add GitHub to supported IDPs firebase/FirebaseUI-iOS#275

Closed

samtstern mentioned this pull request Jun 22, 2018

[Translation] Get translations for Github strings #1365

Closed

Add support for GitHub IDP #1199

Add support for GitHub IDP #1199

Conversation

SUPERCILEX commented Mar 22, 2018 • edited Loading

SUPERCILEX commented Mar 22, 2018

SUPERCILEX commented Jun 13, 2018

samtstern commented Jun 13, 2018

samtstern 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

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

SUPERCILEX commented Jun 18, 2018

SUPERCILEX commented Jun 18, 2018

SUPERCILEX commented Jun 18, 2018

samtstern commented Jun 18, 2018

SUPERCILEX commented Jun 18, 2018

samtstern commented Jun 18, 2018

samtstern commented Jun 18, 2018 • edited Loading

SUPERCILEX commented Jun 18, 2018

SUPERCILEX commented Jun 18, 2018

samtstern commented Jun 18, 2018 • edited Loading

SUPERCILEX commented Jun 18, 2018

samtstern commented Jun 18, 2018

SUPERCILEX commented Jun 18, 2018

samtstern commented Jun 18, 2018 • edited Loading

SUPERCILEX commented Jun 18, 2018

SUPERCILEX commented Jun 19, 2018

samtstern commented Jun 19, 2018

SUPERCILEX commented Jun 19, 2018

samtstern commented Jun 19, 2018

SUPERCILEX commented Jun 19, 2018

SUPERCILEX commented Jun 19, 2018

samtstern commented Jun 19, 2018

SUPERCILEX commented Jun 19, 2018

SUPERCILEX commented Mar 22, 2018 •

edited

Loading

samtstern commented Jun 18, 2018 •

edited

Loading

samtstern commented Jun 18, 2018 •

edited

Loading

samtstern commented Jun 18, 2018 •

edited

Loading