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 idp endpoints to FTW server #1365

Merged
merged 3 commits into from
Oct 12, 2020
Merged

Conversation

OtterleyW
Copy link
Contributor

This PR adds authWithIdp and createWithIdp endpoints to FTW server which enables authenticating and signing up to Flex using access token from 3rd party identity provider (e.g. Facebook or Google)

When done, this branch will be merged to social-logins base branch.

src/util/api.js Show resolved Hide resolved
server/apiRouter.js Outdated Show resolved Hide resolved
@@ -16,6 +16,8 @@ const transactionLineItems = require('./api/transaction-line-items');
const initiatePrivileged = require('./api/initiate-privileged');
const transitionPrivileged = require('./api/transition-privileged');

const createWithIdp = require('./api/auth/createWithIdp');
Copy link
Contributor

Choose a reason for hiding this comment

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

Where's authWithIdp?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or is it omitted on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using authWithIdp only internally in FTW server (e.g. here: https://github.com/sharetribe/ftw-daily/pull/1366/files#diff-c2569a98b269447cc716700472d37356R51) so there is no specific need for adding a route for it.

First I had the authWithIdp under api-util instead of api/auth but that felt also confusing.

// after showing a confirm page to user

res.cookie(
'authinfo',
Copy link
Contributor

Choose a reason for hiding this comment

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

We should document our cookies somewhere. Previously, we had just 2 cookies of our own (sdk & cookie-consent cookie). Then integrations brought their own cookies: Stripe, map provider, etc.

This specific info should be cleared from cookies (at least, on logout)
https://github.com/sharetribe/ftw-daily/blob/master/src/reducers.js

.end();
})
.catch(e => {
handleError(res, e);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to cookie?
(I assume that it should be cleared here (and ensure it also on logout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cookie will be cleared if the authentication is successful or it will expire after 15 min (or this time can be changed).

I'm not sure if the cookie should be cleared here when there is an error because then the user is not able to try the confirm step again. E.g. the error could be a conflicting email and then the user could try again with a different email (but I'm not sure how likely something like this would happen. One scenario would be that user has signed up with the same email as in e.g. Facebook but not verified the email address yet and for some reason they would decide to use different email in Flex).

maxAge: 15 * 60 * 1000, // 15 minutes
}
)
.redirect(`${rootUrl}/login#`);
Copy link
Contributor

@Gnito Gnito Sep 22, 2020

Choose a reason for hiding this comment

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

This is assuming that the web app has a route called "login" and customizers haven't changed the route configuration.

would send work instead -> i.e. let web app worry about its own routing?

server/apiRouter.js Outdated Show resolved Hide resolved
src/util/api.js Outdated Show resolved Hide resolved
@OtterleyW OtterleyW force-pushed the social-logins branch 2 times, most recently from 528af30 to 927c290 Compare October 1, 2020 14:32
@OtterleyW OtterleyW merged commit a5bb186 into social-logins Oct 12, 2020
@OtterleyW OtterleyW deleted the add-idp-endpoints-to-server branch October 12, 2020 13:19
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