Skip to content

Enables login over POST in addition to GET #4268

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

Merged
merged 2 commits into from
Oct 24, 2017

Conversation

montymxb
Copy link
Contributor

@montymxb montymxb commented Oct 20, 2017

So this is a little something for #4052 . The change is relatively minor and really doesn't do anything on it's own. A single test is added to verify login works over POST using REST.

Most importantly this will allow future development on the sdks to eventually switch to using POST for login requests, instead of GET. Perhaps at some distant point in time we could even consider deprecating and eventually removing the GET approach, but for now keeping it is essential for reverse compatibility.

Just to make a point there really isn't anything too bad about the current approach using GET, so long as the connection is secure. The one thing that really stands out is that if logging is not done properly you risk including username/password combos in the log. Although this would really only happen if you were directly piping output from the process into a log or something like that. There is that chance that if you set up logging improperly and someone else has access to that log that you might have a problem. Although this doesn't really fix that this gives us the option to do so down the road.

@codecov
Copy link

codecov bot commented Oct 20, 2017

Codecov Report

Merging #4268 into master will increase coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4268      +/-   ##
==========================================
+ Coverage    92.2%   92.43%   +0.22%     
==========================================
  Files         116      118       +2     
  Lines        8124     8151      +27     
==========================================
+ Hits         7491     7534      +43     
+ Misses        633      617      -16
Impacted Files Coverage Δ
src/Routers/UsersRouter.js 91.12% <100%> (+0.07%) ⬆️
src/Config.js 93.96% <0%> (-1.34%) ⬇️
src/cli/utils/commander.js 100% <0%> (ø) ⬆️
src/index.js 100% <0%> (ø) ⬆️
src/cli/definitions/parse-server.js 100% <0%> (ø) ⬆️
src/cli/utils/parsers.js
src/Options/Definitions.js 100% <0%> (ø)
src/Controllers/index.js 96.66% <0%> (ø)
src/Options/parsers.js 100% <0%> (ø)
src/LiveQuery/ParseLiveQueryServer.js 86.51% <0%> (+0.04%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70ad9e9...462fcf9. Read the comment docs.

'X-Parse-REST-API-Key': 'rest',
},
json: {
_method: 'POST',
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn’t need that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flovilmart shouldn't need to indicate post?

@montymxb
Copy link
Contributor Author

montymxb commented Oct 24, 2017

@flovilmart yeah rp.post will perform POST like it says without an explicit method required, just double checked it to be absolutely certain. Change is up.

@flovilmart
Copy link
Contributor

the _method prop is an internal parse thing so we can send everything as POST queries an interpolate as _method on the server.

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.

2 participants