Integrate auth adapter for Facebook accountkit login #4434

6thfdwp · 2017-12-19T07:10:35Z

First time PR to parse-server, try to learn and contribute back as our app also migrated to open source parse-server since last year. Hope to be part of this community to make it robust.

codecov · 2017-12-20T01:17:25Z

Codecov Report

Merging #4434 into master will decrease coverage by 0.14%.
The diff coverage is 70.58%.

@@            Coverage Diff             @@
##           master    #4434      +/-   ##
==========================================
- Coverage   92.89%   92.74%   -0.15%     
==========================================
  Files         118      119       +1     
  Lines        8445     8479      +34     
==========================================
+ Hits         7845     7864      +19     
- Misses        600      615      +15

Impacted Files	Coverage Δ
src/Adapters/Auth/index.js	`92.3% <100%> (+0.15%)`	⬆️
src/Adapters/Auth/facebookaccountkit.js	`69.69% <69.69%> (ø)`
src/Adapters/Cache/InMemoryCache.js	`91.66% <0%> (-8.34%)`	⬇️
src/RestWrite.js	`93.1% <0%> (-0.73%)`	⬇️
src/Adapters/Auth/meetup.js	`89.47% <0%> (+5.26%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 550b69e...154c088. Read the comment docs.

…server into auth-fbaccountkit

6thfdwp · 2017-12-20T13:16:10Z

commit#1e8112 as required in Accountkit doc:

If you have enabled the Require App Secret setting in your app's dashboards, most calls that accept an account access token as a parameter will now require an additional appsecret_proof parameter to verify that these calls are coming from your own servers.
The app secret proof is a sha256 hash of your access token.

montymxb · 2017-12-21T00:13:20Z

@6thfdwp thanks for the contribution! We're always glad to have new contributors in the community 👍 . I'm rerunning CI now as it looks like your tests are good. I'll follow up when that's done.

montymxb

Looking good for an initial PR 👍 !

Couple of things that we'll need changed.

The error messages should be unique to this auth adapter, wouldn't want to mistake them for the other facebook one.
Code coverage diff should be as close to 100% as possible. In this case you can probably make a test which runs a faulty request through or the like.

Other than that this is looking good. Also it's getting a big close to the holidays so we might be a bit absent here and there 😄 .

montymxb · 2017-12-21T00:24:35Z

src/Adapters/Auth/facebookaccountkit.js

+        }
+      });
+    }).on('error', function () {
+      reject('Failed to validate this access token with Facebook.');


You'll want to customize this error message as it's identical to the one for facebook auth.

montymxb · 2017-12-21T00:24:45Z

src/Adapters/Auth/facebookaccountkit.js

+  if (!appIds.length) {
+    throw new Parse.Error(
+      Parse.Error.OBJECT_NOT_FOUND,
+      'Facebook app id is not configured.');


Facebook app id is the same for Account Kit login, so I'll probably go like 'Facebook app id for Account Kit'

montymxb · 2017-12-21T00:24:51Z

src/Adapters/Auth/facebookaccountkit.js

+      }
+      throw new Parse.Error(
+        Parse.Error.OBJECT_NOT_FOUND,
+        'Facebook app id is invalid for this user.');


montymxb · 2017-12-21T00:25:00Z

src/Adapters/Auth/facebookaccountkit.js

+      }
+      throw new Parse.Error(
+        Parse.Error.OBJECT_NOT_FOUND,
+        'Facebook auth is invalid for this user.');


and also here

6thfdwp · 2017-12-21T00:47:36Z

Thanks for the feedback! I'll do these tweaks.

I've also actually done some local test for full request flow, all good, but this requires real access token and secret which not suitable to put in test case. But surely I can add some faulty request case with bad token etc.

Have a nice holiday! 😀

6thfdwp · 2018-01-23T06:26:04Z

Hi @montymxb @flovilmart,

just check how to make this PR pass CI and other checks, I've done changes based on @montymxb review. I'd like to find new issues/enhancement to work on, hope that I can learn to get workflow right from the first PR for future commits.

Thanks!

montymxb · 2018-02-01T18:03:41Z

Hey @6thfdwp , been a bit absent 😆 , haven't been around for like 2 weeks...

I've reupped to the latest changes in master and we're pending some CI here now. Looks like the last run passed but I just want to verify that this one is good. If it succeeds I'll give this a final pre-flight inspection.

Thanks for pinging us by the way! I track things mostly asynchronously using the notifications we get for the repos. Sometimes previous work can get buried while we're attempting to address what's new for a given week or day. I generally don't touch anything until I can actually respond to it, because as soon as I view anything the notification is cleared. Really wish github would allow us to reflag things for later followup or to make notifications unread.

montymxb

1 typo and something that should help account for untested functionality. We won't be able to get 100% given we would need actual live appIds and a secret, which we're definitely not doing.

If you can make those couple changes that would be super 👍 .

montymxb · 2018-02-01T18:16:56Z

spec/AuthenticationAdapters.spec.js

+      })
+  });
+
+  it('should fail to validate Facebook accountkit auth with bad taken regardless of app secret proof', (done) => {


typo on taken, should be token

montymxb · 2018-02-01T18:21:14Z

src/Adapters/Auth/facebookaccountkit.js

+  if (!appIds.length) {
+    throw new Parse.Error(
+      Parse.Error.OBJECT_NOT_FOUND,
+      'Facebook app id for Account Kit is not configured.');


I would add one more test to help cover this, just omit your appIds and this should trigger.

@montymxb I looked in Adapters/Auth/index.js#L50, seems in real flow, we only do validateAppId if it's provided. So in test case I provided appIds as empty array to simulate it's not properly configured

montymxb · 2018-02-01T18:24:17Z

I would like to add that other than those couple things the code in this PR looks good to me. Once those are addressed I believe we should be ok to approve & merge.

6thfdwp · 2018-02-02T07:49:23Z

@montymxb no worries, totally understand when facing lots of issues and requests.

I've actually used this adapter in our code base, it's been live supporting phone number login. I thought would be good to integrate and make more improvements.

Thanks for review again, I will push once I update.

…gured properly

…server into auth-fbaccountkit

otymartin · 2018-02-16T21:38:16Z

@6thfdwp is this live or dead?
Im looking to integrate accountkit

6thfdwp · 2018-02-20T05:28:56Z

@otymartin seems still under review, waiting to be merged

otymartin · 2018-02-21T21:54:24Z

@flovilmart anyway we can get this approved (assuming it's production ready ) 🙏🙏

flovilmart · 2018-02-21T21:59:41Z

@6thfdwp thanks for the PR, can you please update the docs alongside the PR before we merge this?

https://github.com/parse-community/docs/blob/gh-pages/_includes/parse-server/third-party-auth.md

Thanks!

6thfdwp · 2018-02-23T12:24:50Z

@flovilmart just updated docs. Please review.

Thanks!

flovilmart · 2018-02-23T12:37:29Z

This is looking good! Thanks!

otymartin · 2018-02-23T17:51:33Z

Thanks @6thfdwp

6thfdwp · 2018-02-25T14:23:14Z

Awesome, thanks @flovilmart

Hope it will work, feedback welcomed @otymartin

montymxb · 2018-02-27T01:40:00Z

@6thfdwp thanks for hanging in there! Sorry I wasn't able to finish this up myself, but glad to see you got this in.

* Integrate auth adapter for Facebook accountkit login * Also verify Facebook app id associated with account kit login * Add appsecret_proof as extra graph request parameter * Specific error message for Account kit and more test coverage * One more test to cover when AppIds for Facebook account kit not configured properly

DingWeiping and others added 3 commits December 19, 2017 15:53

Integrate auth adapter for Facebook accountkit login

b488568

Also verify Facebook app id associated with account kit login

b44c378

Merge branch 'master' into auth-fbaccountkit

c41d294

DingWeiping and others added 2 commits December 20, 2017 11:50

Merge branch 'auth-fbaccountkit' of https://github.com/6thfdwp/parse-…

e65054d

…server into auth-fbaccountkit

Add appsecret_proof as extra graph request parameter

1e81124

montymxb suggested changes Dec 21, 2017

View reviewed changes

Specific error message for Account kit and more test coverage

060020a

dplewis and others added 2 commits January 23, 2018 09:17

Merge branch 'master' into auth-fbaccountkit

f74e29d

Merge branch 'master' into auth-fbaccountkit

bd7c242

montymxb added the enhancement label Feb 1, 2018

montymxb suggested changes Feb 1, 2018

View reviewed changes

DingWeiping added 2 commits February 6, 2018 15:35

One more test to cover when AppIds for Facebook account kit not confi…

47b040b

…gured properly

Merge branch 'auth-fbaccountkit' of https://github.com/6thfdwp/parse-…

154c088

…server into auth-fbaccountkit

flovilmart merged commit 213801c into parse-community:master Feb 23, 2018

mtrezza removed the 🧬 enhancement label Jul 11, 2021

Uh oh!

Integrate auth adapter for Facebook accountkit login #4434

Integrate auth adapter for Facebook accountkit login #4434

Uh oh!

Conversation

6thfdwp commented Dec 19, 2017

Uh oh!

codecov bot commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

6thfdwp commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

montymxb commented Dec 21, 2017

Uh oh!

montymxb left a comment

Choose a reason for hiding this comment

Uh oh!

montymxb Dec 21, 2017

Choose a reason for hiding this comment

Uh oh!

montymxb Dec 21, 2017

Choose a reason for hiding this comment

Uh oh!

6thfdwp Dec 21, 2017

Choose a reason for hiding this comment

Uh oh!

montymxb Dec 21, 2017

Choose a reason for hiding this comment

Uh oh!

montymxb Dec 21, 2017

Choose a reason for hiding this comment

Uh oh!

6thfdwp commented Dec 21, 2017

Uh oh!

6thfdwp commented Jan 23, 2018

Uh oh!

montymxb commented Feb 1, 2018

Uh oh!

montymxb left a comment

Choose a reason for hiding this comment

Uh oh!

montymxb Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

montymxb Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

6thfdwp Feb 7, 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!

montymxb commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

6thfdwp commented Feb 2, 2018

Uh oh!

otymartin commented Feb 16, 2018

Uh oh!

6thfdwp commented Feb 20, 2018

Uh oh!

otymartin commented Feb 21, 2018

Uh oh!

flovilmart commented Feb 21, 2018

Uh oh!

6thfdwp commented Feb 23, 2018

Uh oh!

flovilmart commented Feb 23, 2018

Uh oh!

otymartin commented Feb 23, 2018

Uh oh!

6thfdwp commented Feb 25, 2018

Uh oh!

montymxb commented Feb 27, 2018

Uh oh!

Uh oh!

codecov bot commented Dec 20, 2017 •

edited

Loading

6thfdwp commented Dec 20, 2017 •

edited

Loading

6thfdwp Feb 7, 2018 •

edited

Loading

montymxb commented Feb 1, 2018 •

edited

Loading