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

Fixes sessionTokens being overridden in 'find' #4332

Merged

Conversation

montymxb
Copy link
Contributor

@montymxb montymxb commented Nov 8, 2017

This is a fix for #4326 where querying for _Session ends up returning sessions that all contain the same sessionToken. It appears that, at some point earlier on in parse-server, code was added to override any session tokens in all objects returned if there was as sessionToken passed in the request.

Normally this wouldn't be a problem as you wouldn't see any other sessions unless you were using the masterKey, and usually then the SDKs may have an option to omit the sessionToken since it is unneeded at that point. In the case where you pass both you can end up with a situation where you find more than 1 session and all of their tokens are overridden with your own.

As a plus this removes unnecessary looping code that was running all the time on every 'find' request, which may give us a slight performance bump in finds.

This adds a test to verify that this doesn't happen in the future as well, added ParseSession.spec.js since that seemed appropriate given the context.

@montymxb
Copy link
Contributor Author

montymxb commented Nov 8, 2017

While this is pending I noticed some similar code here in the class router during 'get'. It seems suspicious, not sure when the sessionToken would be off on the user. Maybe when they're signed in on multiple devices?

@montymxb
Copy link
Contributor Author

montymxb commented Nov 9, 2017

Scratch that, this test in ParseUser.spec.js wants that other sessionToken section to stay in place.

@codecov
Copy link

codecov bot commented Nov 9, 2017

Codecov Report

Merging #4332 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4332      +/-   ##
==========================================
- Coverage   92.74%   92.71%   -0.03%     
==========================================
  Files         119      119              
  Lines        8438     8434       -4     
==========================================
- Hits         7826     7820       -6     
- Misses        612      614       +2
Impacted Files Coverage Δ
src/Routers/ClassesRouter.js 93.15% <ø> (-0.36%) ⬇️
src/RestWrite.js 93.32% <0%> (-0.37%) ⬇️

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 37ceae0...9c6f6e2. Read the comment docs.

@montymxb montymxb requested a review from flovilmart November 10, 2017 21:06
@montymxb
Copy link
Contributor Author

Just to double check I went ahead and substituted the following code to see if any cases occurred where a substitution by a user without master occur in our tests.

if (response && response.results) {
  for (const result of response.results) {
	if (result.sessionToken && req.info.sessionToken && result.sessionToken !== req.info.sessionToken) {
	  // PERFORM THIS REPLACEMENT ONLY WHEN IT FACTORS IN
	  result.sessionToken = req.info.sessionToken || result.sessionToken;
	}
  }
}

When this change is made nothing hits this, as the only token they ever see is their own. The only case where this would hit is in the provided test case in this PR, where both a sessionToken & masterKey are present. In that case the user is still authorized to get that information anyways.

@flovilmart after checking that out I don't believe there's any issues with bringing this in. Have any thoughts on this?

@flovilmart
Copy link
Contributor

@montymxb can we check some corner cases here, like querying _Session or _User, without masterKey and making sure the sessionToken is not overriden? Also, #4326 does not mention using the masterKey for the query, can we check that particular use case too?

@montymxb
Copy link
Contributor Author

Sure thing. I think we have some of those covered already, but I'll take a look. Anything that hasn't already been checked I'll go ahead and put in here just to make sure we're not missing something we should be adjusting.

@flovilmart flovilmart merged commit 219ad72 into parse-community:master Nov 25, 2017
@montymxb montymxb deleted the session-token-replacement-fix branch November 26, 2017 00:11
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* remove session token replacement code

* adds cases for _User/_Session with sessionToken and with/without masterKey
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