8000 feat(authentication-oauth): Set oAuth redirect URL dynamically by fadiquader · Pull Request #1608 · feathersjs/feathers · GitHub
[go: up one dir, main page]
Skip to content

feat(authentication-oauth): Set oAuth redirect URL dynamically#1608

Merged
daffl merged 4 commits intofeathersjs:masterfrom
fadiquader:master
Nov 27, 2019
Merged

feat(authentication-oauth): Set oAuth redirect URL dynamically#1608
daffl merged 4 commits intofeathersjs:masterfrom
fadiquader:master

Conversation

@fadiquader
Copy link
Contributor
@fadiquader fadiquader commented Oct 8, 2019

Summary

Referred to this bug #1591
Fixed set oauth redirect URL dynamically.

@daffl
Copy link
Member
daffl commented Oct 14, 2019

Thank you! I updated the PR with a test and changed it to pass all of params to the getRedirect method.

The default oAuth strategy now also includes the original redirect setting. So a query redirect like ?redirect=/dashboard with a oauth.redirect option of https://app.mydomain.com will redirect to https://app.mydomain.com/dashboard. The redirect can't be a generic URL, otherwise an attacker could steal a token by sending a link that redirects to their URL.

@daffl daffl mentioned this pull request Nov 27, 2019
Merged
@daffl daffl changed the title Issue#1591 [fix] Set oAuth redirect URL dynamically feat(authentication-oauth): Set oAuth redirect URL dynamically Nov 27, 2019
@daffl daffl merged commit 1293e08 into feathersjs:master Nov 27, 2019
@elv1n
Copy link
elv1n commented Dec 13, 2019

@daffl Redirect only works when featers_token provided to link accounts. It will be extremely useful to have a query in session to manipulate with it in getRedirect fn.
Are there any downsides to have the feature?

@daffl
Copy link
Member
daffl commented Dec 13, 2019

Are you sure? This should work whenever the ?redirect= query parameter is set during oAuth login.

@elv1n
Copy link
elv1n commented Dec 13, 2019

Sorry, if I missed something
Here is a skip part

 if (feathers_token) {
        debug(`Got feathers_token query parameter to link accounts`, feathers_token);
        req.session.accessToken = feathers_token;
        req.session.query = query;
      }

and then it takes from session on authenticate route

 authApp.get('/:name/authenticate', async (req, res, next) => {
      const { name } = req.params as any;
      const { accessToken, grant, query = {} } = req.session;

@daffl
Copy link
Member
daffl commented Dec 14, 2019

Dang, you're totally right, that should definitely live outside of that statement. PR would be welcome.

EliSadaka pushed a commit to yusernetwork/authentication-oauth that referenced this pull request Oct 20, 2020
@fadiquader @daffl
feat(authentication-oauth): Set oAuth redirect URL dynamically (feath…
d7ff49e 
…ersjs#1608)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fadiquader @daffl @elv1n
0