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

Option to append access token to success redirect URL #157

Closed
wants to merge 5 commits into from

Conversation

jeff3yan
Copy link

Added an option for appending the access token as a query parameter on the success redirect URL. Similar to this fork, but with a config option redirectWithToken.

This was desired due to loopback-component-passport 3rd party login only supporting access token setting via cookies, which isn't feasible when the front-end and API are on different domains.

Similar in intent to #102, but it allows continued usage of the returnTo query parameter and is useful for apps that may have dynamic redirects on separate domains.

return res.redirect(successRedirect(req) + (options.redirectWithToken ? '?access_token=' + info.accessToken.id : ''));

@slnode
Copy link

slnode commented May 26, 2016

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@jannyHou
Copy link
Contributor

jannyHou commented Jun 9, 2016

@slnode test please

@matias-casal
Copy link

please, make it

@slnode
Copy link

slnode commented Nov 10, 2016

Can one of the admins verify this patch?

1 similar comment
@slnode
Copy link

slnode commented Nov 10, 2016

Can one of the admins verify this patch?

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

The two options added in pr looks reasonable to me.
@jeff3yan Can you add some test cases for each option? With test case pass on CI we can check in the pr then.
And please also fix the commit message, here is the linting error:

**************************************************
**
**  Linting commit logs
**
**  4 problems found:
**    5c9ced3 - Merge branch 'feature-redirect-with-accesstoken' of: First line should be 50 characters or less (saw 138)
**    38074de - Add option to append access token to success redire: First line should be 50 characters or less (saw 57)
**    26fa7e9 - Allow cookieDomain to be explicitly set to some fal: First line should be 50 characters or less (saw 60)
**    b84d9bb - Add cookieDomain configuration since domain is a re: First line should be 50 characters or less (saw 95)
**
**************************************************

});
}
}
return res.redirect(successRedirect(req));
return res.redirect(successRedirect(req) + (options.redirectWithToken ? '?access_token=' + info.accessToken.id : ''));
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure is it safe to expose accessToken in url, @loay idea?

@stale
Copy link

stale bot commented Aug 23, 2017

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@divsbhalala
Copy link

how can i use this feature ? any doc ?

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.

None yet

9 participants