E528 Fix authentication-oauth regression using incorrect callback and redirect_uri by johnheenan · Pull Request #2631 · feathersjs/feathers · GitHub
[go: up one dir, main page]
Skip to content

Fix authentication-oauth regression using incorrect callback and redirect_uri#2631

Merged
daffl merged 5 commits intofeathersjs:dovefrom
johnheenan:dove
May 23, 2022
Merged

Fix authentication-oauth regression using incorrect callback and redirect_uri#2631
daffl merged 5 commits intofeathersjs:dovefrom
johnheenan:dove

Conversation

@johnheenan
Copy link
Contributor
@johnheenan johnheenan commented May 11, 2022

Fix authentication-oauth regression: authentication callback and redirect_uri were using incorrect protocol and host from configuration

Summary

Please see #2630

Call to defaultsDeep parameters reordered to ensure protocol and host values from both both oauth.defaults and from local variable can be distinguished, with local values as fallback.

Previous version was coding local values instead of oauth.default values. This matters if they are not the same, such as when using reverse proxy server. The local host value is from top host value of configuration file defualt.json.

@johnheenan
Fix authentication-oauth regression: authentication callback and redi…
9dc5010 
…rect_uri were using incorrect protocol and host from configuration

Call to defaultsDeep parameters reordered to ensure protocol and host values from both both oauth.defaults and from local variable can be distinguished, with local values as fallback.

Previous version was coding local values instead of oauth.default values. This matters if they are not the same, such as when using reverse proxy server. The local host value is from top host value of configuration file defualt.json.
@johnheenan johnheenan mentioned this pull request May 11, 2022
Closed
@johnheenan
Copy link
Contributor Author
johnheenan commented May 11, 2022 •
edited
Loading

I should let you know that I tried the following with oauth last, instead of second following {} as in the existing solution, but it did not work. The results are the same as the existing solution. I accept the existing solution , with oath second should work.

const grant = defaultsDeep({}, {
    defaults: {
      prefix,
      origin: `${protocol}://${host}`,
      transport: 'session',
      response: ['tokens', 'raw', 'profile']
    }
  }, omit(oauth, ['redirect', 'origins']));

This is the suggested solution that works, but it should have an addition.

const grant = defaultsDeep({
    defaults: {
      prefix,
      origin: `${oauth?.defaults?.protocol ?? protocol}://${oauth?.defaults?.host ?? host}`,
      transport: 'session',
      response: ['tokens', 'raw', 'profile']
    }
  }, omit(oauth, ['redirect', 'origins']));

The following is better to ensure the transport from the configuration is used. I will add another commit to the PR.

  const grant = defaultsDeep({
    defaults: {
      prefix,
      origin: `${oauth?.defaults?.protocol ?? protocol}://${oauth?.defaults?.host ?? host}`,
      transport:  oauth?.defaults?.transport ?? 'session',
      response: ['tokens', 'raw', 'profile']
    }
  }, omit(oauth, ['redirect', 'origins']));

@daffl
Copy link
Member
daffl commented May 22, 2022

Thank you for the pull request! When comparing it to v4 I'm thinking the order is just wrong which is what you changed but then we can keep the old host settings. I'll quickly update and it will get out with the new release tomorrow and you can let me know if that fixes it.

@johnheenan
Copy link
Contributor Author
johnheenan commented May 23, 2022

Thanks for the commit. No, changing the order does not fix the problem with my config/default.json. My fix does fix the problem with my config. I thoroughly tested your and my version, to the extent of checking the compiled module js.

We need to step back a bit because we may be at cross purposes given the way I configure feathers for authentication. I need to state how I configure and why I configure this way. These reasons, with a sample configuration, are on #2630, so you and others can comment on this if they wish.

@daffl
Copy link
Member
daffl commented May 23, 2022

No my bad, you were correct. I thought it would still fill in the correct settings. Will merge your original PR and put it out with the next release shortly.

@daffl daffl merged commit 43d8a08 into feathersjs:dove May 23, 2022
@cciocov cciocov mentioned this pull request Jun 7, 2022
Merged
@cciocov
Copy link
Contributor
cciocov commented Jun 7, 2022

@johnheenan @daffl
This merge introduces a bug, in that you are no longer able to override the keys under "defaults". SInce we're using "defaultsDeep" instead of "merge", the order of parameters passed should be inverse (i.e.: higher priority parameters first, which are the ones coming in the "oauth" object).

Opened PR #2659 to fix this.

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

@johnheenan @daffl @cciocov
0