feathersjs-ecosystem / authentication-oauth2

[MOVED] OAuth 2 plugin for feathers-authentication
https://github.com/feathersjs/feathers
MIT License
26 stars 15 forks source link

Added support for redirect options on strategy options (#41) #42

Closed nsainaney closed 7 years ago

nsainaney commented 7 years ago

Fix for issue #41

daffl commented 7 years ago

Thank you @nsainaney. Should this be called oauth2Settings.redirectOptions or just pass oauth2Settings? It looks like @nabeards expected it to be added at the top level (as properties on oauth2Settings) which I think should be fine as long as it doesn't error (or conflict with other settings).

nsainaney commented 7 years ago

Simplified the options based on feedback from @daffl and @nabeards

ekryski commented 7 years ago

this looks good. Thank you @nsainaney! :shipit:

nsainaney commented 7 years ago

%$^ - I'll fix. Easy enough

@ekryski commented on this pull request.


In test/index.test.jshttps://github.com/feathersjs/feathers-authentication-oauth2/pull/42#discussion_r140864756:

@@ -112,6 +112,19 @@ describe('feathers-authentication-oauth2', () => { app.passport.options.restore(); });

Should say "redirect". Not a show stopper....

nsainaney commented 7 years ago

Fixed typo in test name

nsainaney commented 7 years ago

Ugh - I believe I've made a mistake. The config is erroneously:

{
auth : {
   github: {
       // clientId etc
      github : {
      // redirect options
     }
  }
}

instead of

{
auth : {
   github: {
       // clientId etc
      // redirect options
  }
}

I didn't realize this until I put it into a real project. Any objections to re-opening and letting me fix this?

My apologies

daffl commented 7 years ago

Yes. Please submit another PR and we'll make a patch release.

nsainaney commented 7 years ago

See PR #44