Skip to content
This repository has been archived by the owner on Sep 11, 2018. It is now read-only.

Fix for Update an existing entity in verifier #18 #86

Merged
merged 4 commits into from Aug 28, 2018
Merged

Fix for Update an existing entity in verifier #18 #86

merged 4 commits into from Aug 28, 2018

Conversation

nsainaney
Copy link
Contributor

@nsainaney nsainaney commented Aug 16, 2018

This is a cleaner fix for issue #18 and does not require changes to any existing code. The user will, however, have to enable cookies for social linking to work and add cookie-parser

Copy link
Member

@daffl daffl left a comment

Choose a reason for hiding this comment

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

This is great, thank you! One comment but I'm looking forward to getting this released.

lib/index.js Outdated
@@ -73,6 +87,7 @@ function init (options = {}) {
app.get(oauth2Settings.path, auth.express.authenticate(name, oauth2Settings));
app.get(
Copy link
Member

Choose a reason for hiding this comment

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

Would it be safe to just add the cookieParser middleware here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it would, however, social linking would not work if cookies aren't enabled by the developer for @feathersjs/authentication-jwt.

Copy link
Member

Choose a reason for hiding this comment

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

I think that should be fine, since you need to do that when using the client with oAuth2 anyway (the generator enables cookies automatically in that case already).

@daffl daffl mentioned this pull request Aug 28, 2018
@daffl
Copy link
Member

daffl commented Aug 28, 2018

Actually just realized that this will work through feathersjs-ecosystem/authentication-jwt#55 if cookies are enabled, no cookie parser necessary. Thank you again, this is great work!

@daffl daffl merged commit 9a3d27c into feathersjs-ecosystem:master Aug 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants