JS SDK: Facebook/Google OAuth Redirect #145

jsflax · 2018-06-21T03:05:39Z

No description provided.

…o STITCH-1530b

coveralls · 2018-06-21T03:55:02Z

Coverage increased (+0.5%) to 80.992% when pulling 9193cb2 on jsflax:STITCH-1530b into 6fa9627 on mongodb:master.

edaniels

Nice work so far. Have some changes to make for this pass. This still needs unit tests as well.

edaniels · 2018-06-21T03:18:44Z

packages/browser/core/src/core/auth/internal/StitchBrowserAppAuthRoutes.ts

+   * @returns {string} The encoded object
+   */
+  private uriEncodeObject(obj: object): string {
+    return encodeURIComponent(base64.btoa(JSON.stringify(obj)));


[remove] can you please use b64DecodeUnicode from JWT.ts. Feel free to move it somewhere else. Also remove the base64 package from our deps. That one you included isn't good for us AFAIK

edaniels · 2018-06-21T03:49:49Z

packages/browser/core/package.json

  ],
  "license": "Apache-2.0",
  "dependencies": {
+    "Base64": "^1.0.1",


edaniels · 2018-06-21T03:52:24Z

packages/core/sdk/src/auth/providers/internal/StitchAuthResponseCredential.ts

+    /**
+     * The contents of this credential as they will be passed to the Stitch server.
+     */
+    public constructor(public readonly authInfo: AuthInfo) {


[q] does this just create a property and set it?

Yes, similar to how kotlin's data classes function

edaniels · 2018-06-21T03:52:57Z

packages/core/sdk/src/auth/internal/CoreStitchAuth.ts

+        newAuthInfo = credential.authInfo;
+      } else {
+        if (!response || !response.body) {
+          throw new StitchError("response was undefined");


Is there a more specific error you can use? This needs a better error message as it's pretty vague as to where this is coming from

yeah, this should be a StitchServiceError with an unknown error code.

edaniels · 2018-06-21T03:53:58Z

packages/core/sdk/src/auth/internal/CoreStitchAuth.ts

   * requesting the user profile in a separate request.
   */
-  private processLoginResponse(
+  private processLogin(


I think this name should still stay

In addition to that, we should actually pass a response here and we should instead of creating an auth info in the credential, have the response body marshaled.

Perhaps a better solution is to have this method stay as processLogin but have it accept an ApiAuthInfo or AuthInfo instead of a Response, and move the response decoding logic to a method called processLoginResponse which accepts a non-optional Response. That way, the codepaths for the non-redirect logins can stay the same, whereas the link code path can call processLogin directly with the auth info it reconstructs from the URL. The credential would then only need to hold the provider type and name. Thoughts @edaniels ?

👍 handled offline

edaniels · 2018-06-21T04:19:48Z

packages/browser/core/src/core/auth/internal/StitchAuthImpl.ts

+      );
+    }
+
+    if (!redirectState.found) {


all errors below here need better error messages and removal of StitchClient

edaniels · 2018-06-21T04:21:25Z

packages/browser/core/src/core/auth/internal/StitchAuthImpl.ts

+
+    // If we get here, the state is valid - set auth appropriately.
+    return this.loginWithCredentialInternal(
+      new StitchAuthResponseCredential(redirectState.ua)


I'm not sure this will completely work as written. You're missing the provider type and provider name, both of which need to be stored and retrieved (only one is stored). Please add a unit test that proves this.

We should convert the ua here into the servers JSON form. An encode on ApiAuthInfo (ApiAuthInfo.toJSON) would work. Add a unit test for it's encoding.

The conversion should probably be done within CoreStitchAuth. Browser should be agnostic to ApiAuthInfo

Sure that makes sense

edaniels · 2018-06-21T04:22:04Z

packages/core/sdk/src/auth/providers/internal/StitchAuthResponseCredential.ts

+     */
+    public readonly material: { [key: string]: string };
+
+    /**


The contents aren't really passed to the stitch server. AuthInfo is a result to clients, not to the server. we only send access and refresh tokens back.

This wasn't updated

edaniels · 2018-06-21T04:27:54Z

packages/browser/core/src/core/auth/internal/StitchUserImpl.ts

+    credential: StitchRedirectCredential
+  ): Promise<void> {
+    this.auth.setRedirectFragment(
+      RedirectFragmentFields.LinkUser,


I don't get why this is being set. This should eliminate the need for the StitchUserCodec and all changes made in in StitchUser.ts

edaniels · 2018-06-21T04:29:44Z

packages/browser/core/src/core/auth/internal/StitchAuthImpl.ts

+
+  const state = generateState();
+  auth.setRedirectFragment(RedirectFragmentFields.State, state);
+  auth.setRedirectFragment(RedirectKeys.Type, redirectType);


It looks like this isn't ever used again

adamchel

Seems really close, I agree with most of Eric's comments, but I disagreed in a few places. Let's maybe discuss in person when we're all in the office?

adamchel · 2018-06-21T14:10:56Z

packages/browser/core/src/core/auth/internal/StitchAuthImpl.ts

+    );
+  }
+
+  public hasRedirect(): boolean {


I would disagree with you @edaniels . by default, the login/link with redirect redirects to the page from which it was called, so if we don't have this method, I feel like we should get rid of that default behavior and force people to pick a redirect destination as part of the login/link method. (EDIT: Basically what I'm saying is we should make the redirect URL in FacebookRedirectCredential and GoogleRedirectCredential non-optional, if we're getting rid of hasRedirect(). I still think we should have it though, since the 3.0 SDKs handled redirects back to the same page just fine.)

adamchel · 2018-06-21T14:25:40Z

packages/core/sdk/src/auth/internal/CoreStitchAuth.ts

+        newAuthInfo = credential.authInfo;
+      } else {
+        if (!response || !response.body) {
+          throw new StitchError("response was undefined");


yeah, this should be a StitchServiceError with an unknown error code.

adamchel · 2018-06-21T14:36:04Z

packages/core/sdk/src/auth/internal/CoreStitchAuth.ts

   * requesting the user profile in a separate request.
   */
-  private processLoginResponse(
+  private processLogin(


Perhaps a better solution is to have this method stay as processLogin but have it accept an ApiAuthInfo or AuthInfo instead of a Response, and move the response decoding logic to a method called processLoginResponse which accepts a non-optional Response. That way, the codepaths for the non-redirect logins can stay the same, whereas the link code path can call processLogin directly with the auth info it reconstructs from the URL. The credential would then only need to hold the provider type and name. Thoughts @edaniels ?

edaniels · 2018-06-21T15:25:46Z

packages/browser/core/src/core/auth/internal/StitchAuthImpl.ts

+
+    if (!window.location || !window.location.hash) {
+      cleanup();
+      return Promise.reject(new StitchRedirectError("window location hash was undefined"));


we should return a null user.

edaniels · 2018-06-21T15:26:22Z

packages/browser/core/src/core/auth/internal/StitchAuthImpl.ts

+      ourState
+    );
+
+    if (redirectState.lastError || (redirectState.found && !redirectProvider)) {


if we found no redirect info related or stitch, this should return null. specifically, it should look for client app id which ill add to the server now

edaniels · 2018-06-21T18:03:36Z

packages/browser/core/src/core/auth/internal/StitchUserImpl.ts

+    const { redirectUrl, state } = prepareRedirect(
+      this.auth,
+      credential,
+      RedirectTypes.Login


this is the wrong type but I think you don't even need this enum

jsflax · 2018-06-21T20:02:21Z

@adamchel @edaniels PTAL

edaniels

Looking very nice. Just have a bunch of trivial nits. Will re-review once tests are in 😎

edaniels · 2018-06-21T21:59:42Z

package-lock.json

                "long": "^3.2.0"
            }
        },
+        "Base64": {


[shrug] You need to remove this manually or rm -rf node_modules and npm install again.

edaniels · 2018-06-21T22:00:02Z

packages/browser/core/src/core/auth/StitchAuth.ts


+  loginWithRedirect(credential: StitchRedirectCredential): void;
+
+  handleRedirectResult(): Promise<StitchUser>;


Where did hasRedirectResult go?

edaniels · 2018-06-21T22:00:26Z

packages/browser/core/src/core/auth/internal/RedirectFragmentFields.ts

+    ClientAppId = "_stitch_client_app_id",
+}
+
+export default RedirectFragmentFields;


[nit] newline

edaniels · 2018-06-21T22:02:43Z

packages/browser/core/src/core/auth/internal/StitchAuthImpl.ts

-    authRoutes: StitchAuthRoutes,
-    storage: Storage,
-    appInfo: StitchAppClientInfo
+    public readonly browserAuthRoutes: StitchBrowserAppAuthRoutes,


[nit] private

edaniels · 2018-06-21T22:03:55Z

packages/browser/core/src/core/auth/internal/StitchAuthImpl.ts

+    );
+  }
+
+  public linkWithRedirect(


[nit] linkUserWithRedirectInternal Users aren't supposed to call this directly

edaniels · 2018-06-21T22:17:14Z

packages/core/sdk/src/auth/internal/models/StoreAuthInfo.ts


 import { Storage } from "../../../internal/common/Storage";

 import AuthInfo from "../AuthInfo";


Add some simple encode decode tests for this por favor.

edaniels · 2018-06-21T22:17:55Z

packages/core/sdk/src/auth/internal/CoreStitchAuth.ts

    return this.doGetUserProfile()
      .then(profile => {
-        // readonlyly set the info and user
+        // readonly set the info and user


what's readonly?

edaniels · 2018-06-21T22:19:45Z

packages/browser/examples/basic/src/app/index.jsx

-        let userPassClient = client.auth.getProviderClient(UserPasswordAuthProviderClient.factory)
+    componentDidMount() {
+        const client = Stitch.initializeDefaultAppClient(
+            "test-js-sdk-eocey",


edaniels · 2018-06-21T22:20:11Z

packages/core/sdk/src/auth/internal/CoreStitchAuth.ts

+  private processLoginResponse(credential: StitchCredential, response: Response): Promise<TStitchUser> {
+    try {
+      if (!response) {
+        throw new StitchServiceError("response was undefined");


[nit] add some context to this error

edaniels · 2018-06-21T22:20:57Z

packages/browser/coretest/__tests__/auth/StitchAuthImplUnitTests.ts

+);
+
+describe("StitchAuthImpl", () => {
+    it("should handleRedirect", async () => {


This test should be updated or go away after your tests.

jsflax · 2018-06-22T02:07:01Z

@edaniels PTAL

edaniels · 2018-06-22T03:19:10Z

packages/core/sdk/src/auth/internal/CoreStitchAuth.ts

      if (!response) {
-        throw new StitchServiceError("response was undefined");
+        throw new StitchServiceError(
+          `the login response could not be processed for credential: ${credential};` +


[minor] don't log the credential. may contain sensitive info.

edaniels · 2018-06-22T03:22:13Z

packages/browser/coretest/__tests__/auth/StitchAuthImplUnitTests.ts

-        expect((credential as StitchAuthCredential).authInfo).toEqual(redirectResult.ua);        
+        expect(credential.providerName).toEqual("test_provider_name");
+        expect(credential.providerType).toEqual("test_provider_type");
+        expect((credential as StitchAuthCredential).authInfo).toEqual(redirectFragmentResult);


[request] go a little plus ultra here and mock the profile parts and verify you are logged in with the info above

edaniels

LGTM. Just have two requests for you.

adamchel

A few nits but LGTM. We're so close to being done!

adamchel · 2018-06-22T04:07:12Z

packages/browser/core/src/core/auth/internal/StitchRedirectError.ts

+
+export default class StitchRedirectError extends StitchError {
+    constructor(msg: string) { super(msg); }
+}


[nit] newline

adamchel · 2018-06-22T04:10:25Z

packages/core/sdk/__tests__/auth/internal/CoreStitchAuthUnitTests.ts

    });

-    let hasBeenCalled = false;
+    const hasBeenCalled = false;


[nit] do you actually use this anywhere? I think you can get rid of it

adamchel · 2018-06-22T04:13:06Z

packages/core/sdk/src/auth/internal/CoreStitchAuth.ts

  }

  /**
   * Processes the response of the login/link request, setting the authentication state if appropriate, and


[nit] move this comment to processLoginResponse, and if you have time add a comment here describing what this function does now

JS SDK: Facebook/Google OAuth Redirect

fb5063e

jsflax requested review from adamchel and edaniels and removed request for edaniels June 21, 2018 03:05

jsflax added 4 commits June 20, 2018 23:07

Merge branch 'master' into STITCH-1530b

61894a4

Merge branch 'master' of https://github.com/mongodb/stitch-js-sdk int…

ac58939

…o STITCH-1530b

Merge branch 'master' of https://github.com/mongodb/stitch-js-sdk int…

0c96d52

…o STITCH-1530b

Fixed merge issues

cbdfbf1

Add bson

9d747f6

edaniels suggested changes Jun 21, 2018

View reviewed changes

adamchel suggested changes Jun 21, 2018

View reviewed changes

edaniels reviewed Jun 21, 2018

View reviewed changes

jsflax added 5 commits June 21, 2018 14:46

Addressed remarks

ff8c8c3

better error msging

97dd023

moved error to own class

e68c397

More comments, fixed parsing Store objects issue

b496676

fixed auth storage issues

cac99b1

jsflax added 2 commits June 21, 2018 16:35

few more fixes

025358a

fixed broken error handling

130bd20

edaniels reviewed Jun 21, 2018

View reviewed changes

Addressed comments, added tests

9369b56

edaniels reviewed Jun 22, 2018

View reviewed changes

edaniels approved these changes Jun 22, 2018

View reviewed changes

adamchel approved these changes Jun 22, 2018

View reviewed changes

jsflax added 2 commits June 22, 2018 10:22

Fixed tests

8ef6238

Merge from master, prettied, linted

9193cb2

jsflax merged commit 5728943 into mongodb:master Jun 22, 2018


		loginWithRedirect(credential: StitchRedirectCredential): void;

		handleRedirectResult(): Promise<StitchUser>;


		import { Storage } from "../../../internal/common/Storage";

		import AuthInfo from "../AuthInfo";

JS SDK: Facebook/Google OAuth Redirect #145

JS SDK: Facebook/Google OAuth Redirect #145

Uh oh!

Conversation

jsflax commented Jun 21, 2018

Uh oh!

coveralls commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edaniels left a comment

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!

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!

edaniels Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamchel left a comment

Choose a reason for hiding this comment

Uh oh!

adamchel Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

coveralls commented Jun 21, 2018 •

edited

Loading

edaniels Jun 21, 2018 •

edited

Loading

adamchel Jun 21, 2018 •

edited

Loading