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

body-parser needs to be a dependency. #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

body-parser needs to be a dependency. #2

wants to merge 2 commits into from

Conversation

vangorra
Copy link

@vangorra vangorra commented Feb 2, 2016

Adding body-parser as a dependency and configuring it. Also untracking .npm directory, which really shouldn't be tracked.

@rodrigok
Copy link
Member

rodrigok commented Feb 3, 2016

@vangorra what happens if already there is one body parser before?

@vangorra
Copy link
Author

vangorra commented Feb 3, 2016

An error saying cannot read property token of undefined.

@rodrigok
Copy link
Member

rodrigok commented Feb 3, 2016

So, maybe you should check if a body parser exists or turn this optional, right?

@vangorra
Copy link
Author

vangorra commented Feb 3, 2016

Optional, no. The README for node-oauth2-server demonstrates that the npm body-parser package is required. The existing source for this package doesn't include it.

@rodrigok
Copy link
Member

rodrigok commented Feb 3, 2016

If some user is trying to put this package as a middleware that already have the body parser implemented he will have 2 body parsers trying to parse the same body.

The question is, if we have 2 body parsers trying to decode an url encoded body what will happen?

@vangorra
Copy link
Author

vangorra commented Feb 3, 2016

In this case, it doesn't looks like it will matter considering the body parser is being attached to the "app" which is a new instance of express. So the body parsing would be isolated to the routing the "app" variable handles.

@rodrigok
Copy link
Member

rodrigok commented Feb 3, 2016

Yes, but think about something like:

var bodyParser = Npm.require('body-parser');

WebApp.rawConnectHandlers.use(bodyParser.urlencoded({ extended: false}))
WebApp.rawConnectHandlers.use(oauth2server.app);

Will decode the body 2 times.

I'm saying that because you can attach this package to another libraries like JsonRoutes.Middleware.use(oauth2server.app)

@vangorra
Copy link
Author

vangorra commented Feb 3, 2016

Hmm.. I see your concern.

Time to re-evaluate the problem:
The whole issue is caused by oauthserver.coffee initializing routes for get/post on the /authorize url. This approach stuck me as a bit odd for meteor anyway. Meteor does behave like traditional webservers where the page is rendered and sent to the browser. Instead, it distributes the code necessary to render the site and meteor handles two-way data communication with the server. So the concept of having a post-back url handler (@app.post '/oauth/authorize' ...) on a meteor server to handle user data entry is odd.
A more meteor-like solutions would be to remove the authorize url configurations and handle the authorization of client access with a meteor method. The trouble with this approach is the oauth2server (npm module) relies heavily on manipulating an express object for redirects and such. This would be worked around by giving the module mocked object or rolling a fork of the module.

@rodrigok
Copy link
Member

rodrigok commented Feb 3, 2016

The node-oauth2-server https://github.com/thomseddon/node-oauth2-server/pull/203?ts=2 project is being refactored to remove the express dependency, so we can improve that in the future, now this solve our main problems.

@engelgabriel
Copy link
Member

@rodrigok and @vangorra the refactor has been merged! :)

Time to re-evaluate the problem again?

@sampaiodiego
Copy link
Member

wow, this is old!

do you guys have something new to say? or may I close this?

@CLAassistant
Copy link

CLAassistant commented Jul 29, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants