Add Facebook login #1366

OtterleyW · 2020-09-22T11:39:27Z

Adds Facebook login to FTW

This is how the authentication flow works in general:

(these graphs are also in Whimsical Socia logins & SSO board https://whimsical.com/GcZr7nhZwex4WfYgAuiHFG)

If a new user needs to be created there is a new ConfirmSignupForm on ConfirmPage to confirm the data fetched from e.g. Facebook:

This PR can be merged after #1365 is merged to the base branch #1364

OtterleyW · 2020-10-02T10:51:11Z

server/api/auth/loginWithIdp.js

+    res
+      .cookie(
+        'st-autherror',
+        {
+          status: err.status,
+          code: err.code,
+          message: err.message,
+        },
+        {
+          maxAge: 15 * 60 * 1000, // 15 minutes
+        }
+      )


There are two new cookies st-autherror and st-auhtinfo in this flow and @Gnito mentioned that it would be good to document the cookies we use in some place. I'm not sure what would be the best place to do this? E.g. add article to Flex docs or something else

I'd vote Flex Docs.

src/config.js

OtterleyW · 2020-10-02T11:00:03Z

src/containers/AuthenticationPage/AuthenticationPage.js

      tab,
      sendVerificationEmailInProgress,
      sendVerificationEmailError,
      onResendVerificationEmail,
      onManageDisableScrolling,
+      showSocialLoginsForTests,


The snapshot test for AuthenticationPage caused some issues because in the local environment facebook app id was in environment variables and in CI it was not. Because of this, the social login button was added to snapshot when running tests locally but in CI the page was expected to be rendered without the social login button.

Adding extra prop feels really hacky but I'm not sure in what another way we could ensure that the page is rendered similarly locally and in CI. Also with the prop, it was possible to test both options.

Do you have any better ideas on how to handle the snapshot test (or do we even need to keep it)?

Yeah, I agree, this feels a bit hacky.

Jest should automatically define NODE_ENV to be 'test'.
So, try adding the Facebook app id to .env.test (maybe as empty)

src/containers/ListingPage/ListingPage.js

src/util/api.js

server/apiRouter.js

server/index.js

src/containers/AuthenticationPage/AuthenticationPage.js

Gnito · 2020-10-05T23:57:26Z

src/containers/AuthenticationPage/AuthenticationPage.js

+    ) : null;
+
+    const formContent = isConfirm ? (
+      <div className={css.content}>


One thing to consider in this kind of ternary-chain is that should one or more of these be subcomponent (inside this directory) to improve readability.

(And I'm not saying that this is necessarily hard to read, but in Github, it's a bit confusing due to partially visible content.)

server/api/auth/facebook.js

Gnito

I added some comments.
I'm not sure I'm completely aware of all the aspects, but I still think we should parameterize all the possible return paths.

src/translations/en.json

…ng user

… other small UI fixes

OtterleyW force-pushed the add-idp-endpoints-to-server branch from 75d08d9 to e5d7fc3 Compare September 22, 2020 11:42

OtterleyW force-pushed the add-facebook-login branch from a8de9ec to b4eb179 Compare September 22, 2020 11:46

OtterleyW force-pushed the add-idp-endpoints-to-server branch 2 times, most recently from 85bec53 to 5e1d608 Compare October 1, 2020 14:36

OtterleyW force-pushed the add-facebook-login branch from b4eb179 to 3d6099a Compare October 1, 2020 14:38

OtterleyW changed the base branch from add-idp-endpoints-to-server to social-logins October 2, 2020 10:47

OtterleyW commented Oct 2, 2020

View reviewed changes

OtterleyW requested a review from Gnito October 2, 2020 11:12

OtterleyW added the review wanted label Oct 2, 2020

OtterleyW requested a review from lyyder October 2, 2020 11:12

lyyder temporarily deployed to sharetribe-starter-app October 5, 2020 10:54 Inactive

rap1ds temporarily deployed to sharetribe-starter-app October 5, 2020 12:22 Inactive