Skip to content
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

Add Google login to FTW #1376

Merged
merged 13 commits into from
Nov 16, 2020
Merged

Add Google login to FTW #1376

merged 13 commits into from
Nov 16, 2020

Conversation

OtterleyW
Copy link
Contributor

@OtterleyW OtterleyW commented Oct 29, 2020

This PR adds Google login to FTW

It follows the same logic as Facebook login and uses the createUserWithIdp and loginWithIdp endpoints which were added with Facebook login. You can find more information from Facebook login PR #1366

@OtterleyW OtterleyW force-pushed the google-login branch 3 times, most recently from 6b89b4d to 0167d18 Compare November 2, 2020 14:01
@OtterleyW OtterleyW changed the title WiP: Add Google login to FTW Add Google login to FTW Nov 2, 2020
@@ -99,7 +99,9 @@ module.exports = (err, user, req, res, clientID, idpId) => {
}
}
})
.catch(() => {
.catch(err => {
console.error(err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of data the 'err' contains? Is there something that should not end up to logs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe all the console.logs in the server routes could be checked and potentially enforce the usage of log.js instead?

Copy link
Contributor

@lyyder lyyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍. Only suggestion is to try the login once without the client secret and see if a) the login succeeds b) fetching user profile succeeds.

const PORT = parseInt(process.env.REACT_APP_DEV_API_SERVER_PORT, radix);
const rootUrl = process.env.REACT_APP_CANONICAL_ROOT_URL;
const clientID = process.env.REACT_APP_GOOGLE_CLIENT_ID;
const clientSecret = process.env.GOOGLE_CLIENT_SECRET;
Copy link
Contributor

@lyyder lyyder Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering what this is used for. The Google Web login reference does not mention using the client secret: https://developers.google.com/identity/sign-in/web/reference. Apparently this is used to fetch user profile info but shouldn't the access token suffice for that?

What if this is left out? Does fetching the profile fail?

Copy link
Contributor Author

@OtterleyW OtterleyW Nov 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this without clientSecret and it didn't work. At least the Passport.js expects that the client secret is there. After some investigation I think I found the answer from Google's docs: For example, a JavaScript application does not require a secret, but a web server application does.

It's a little confusing because we are also using JavaScript in the FTWs server. But anyway, when we don't use Google's JavaScript client directly for the web application but we are handling the authentication in the web server, we need to provide the client secret too.

@OtterleyW OtterleyW temporarily deployed to sharetribe-starter-app November 12, 2020 14:31 Inactive
@OtterleyW OtterleyW temporarily deployed to sharetribe-starter-app November 12, 2020 14:56 Inactive
@lyyder lyyder temporarily deployed to sharetribe-starter-app November 12, 2020 19:31 Inactive
@OtterleyW OtterleyW merged commit 10f9574 into master Nov 16, 2020
@OtterleyW OtterleyW deleted the google-login branch November 16, 2020 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants