Throw error when setting authData to null #6154

mtrezza · 2019-10-23T02:24:32Z

Fixed authData field in DB overwriting synthesized authData field that contains values of _auth_data_* fields.

Fixed only for mongoDB & Postgres.

dplewis · 2019-10-23T02:27:11Z

Looks good. Can you add a test case?

codecov · 2019-10-23T02:37:30Z

Codecov Report

Merging #6154 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6154      +/-   ##
==========================================
- Coverage   93.76%   93.75%   -0.01%     
==========================================
  Files         166      166              
  Lines       11271    11291      +20     
==========================================
+ Hits        10568    10586      +18     
- Misses        703      705       +2

Impacted Files	Coverage Δ
src/Adapters/Storage/Mongo/MongoTransform.js	`88.67% <100%> (+0.21%)`	⬆️
src/RestWrite.js	`93.42% <100%> (-0.48%)`	⬇️
src/Routers/FilesRouter.js	`92.72% <0%> (-0.26%)`	⬇️
src/Adapters/Files/FilesAdapter.js	`100% <0%> (ø)`	⬆️
src/ParseServer.js	`97.59% <0%> (+0.05%)`	⬆️
src/Adapters/Files/GridFSBucketAdapter.js	`68.85% <0%> (+0.51%)`	⬆️
src/Controllers/FilesController.js	`93.02% <0%> (+0.52%)`	⬆️
src/Adapters/Files/GridStoreAdapter.js	`46.37% <0%> (+0.78%)`	⬆️

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 ac4d9ed...a02fcd2. Read the comment docs.

mtrezza · 2019-10-23T03:25:41Z

Actually it would be better to test this on a higher level instead of MongoTransform, so the test would be automatically done for mongoDB and Postgres, right?

dplewis · 2019-10-23T03:28:03Z

Yes, a REST call will work. I’ll do some additional testing on my end.

mtrezza · 2019-10-23T15:49:21Z

Added a test on ParseUser that reproduces the exact same error:

spec/ParseUser.spec.js

mtrezza · 2019-10-23T17:13:16Z

Is there a way I can easily execute the test locally with Postgres? I only see the failing tests once I update the PR.

dplewis · 2019-10-23T17:23:59Z

Checkout the Contribution guide

https://github.com/parse-community/parse-server/blob/master/CONTRIBUTING.md#run-your-tests-against-postgres-optional

dplewis · 2019-10-27T03:15:50Z

@mtrezza @acinader Instead of ignoring should we throw and error similar to user.unset('authData').

The goal here is to prevent directly manipulating authData in favor of unlink and link

mtrezza · 2019-10-27T12:10:10Z

@dplewis Good point, throwing would create more awareness that there is something wrong than ignoring. Currently the server has an uncaught internal exception, so the benefit of this PR would be a proper error description.

_{Sent with GitHawk}

dplewis · 2019-10-27T20:13:59Z

@mtrezza @acinader How does this look?

mtrezza · 2019-10-27T20:37:02Z

@dplewis

can’t see properly on mobile, but does this only throw for auth in _User class? Should be allowed for other classes.
appears to me that the changes correctly disallow setting authData, but an existing authData field in the _User class will again throw an uncaught internal exception. So how about adding those cases again and throwing when there’s an authData field?

_{Sent with GitHawk}

dplewis · 2019-10-27T20:42:09Z

does this only throw for auth in _User class?

Yes here checks if user class or not

an existing authData field in the _User class will again throw

I don't believe it will throw again. This is a fail fast solution, doesn't matter what's in the DB. Can you write a failing test just in case?

mtrezza · 2019-10-27T21:26:09Z

@dplewis The test you removed from MongoTransform would be the failing test. Can you try with that enabled? I believe it will fail. The issue was that in the transform, the authData field from the DB overwrites the synthesized authData field for the User object. So I believe we should add the case again and throw right there in the tranform for Mongo and Postgres.

If that doesn’t make much sense I can add the changes when I’m not on mobile.

Regarding backwards compatibility, I believe it wouldn’t be worse than the current internal server exception if we throw there.

_{Sent with GitHawk}

dplewis · 2019-10-27T21:37:42Z

@mtrezza Oh I get what you are saying. My fix prevents setting authData. Your fix checks if authData field already exists and prevents it from overriding.

mtrezza · 2019-10-27T21:41:02Z

@dplewis Correct, I believe adding both makes sense. We could also ignore and log a warning when authData exists. Is that something we usually do for that level of severity?

_{Sent with GitHawk}

dplewis · 2019-10-27T23:08:00Z

@mtrezza I added back your changes and wrote more tests.

it is forbidden to add a custom field name beginning with `_`, so if the object is not `_User` , the transform should throw

mtrezza · 2019-10-28T00:28:31Z

@dplewis I think we are getting close :)

acinader

this looks good.

src/Adapters/Storage/Mongo/MongoTransform.js

mtrezza

(fighting with the review feature)

src/Adapters/Storage/Mongo/MongoTransform.js

dplewis · 2019-10-28T01:28:56Z

@mtrezza Nice working with you, we should do this again sometime. 👍

mtrezza · 2019-10-28T01:42:11Z

@dplewis I agree, was my pleasure.

* added ignore authData field * add fix for Postgres * add test for mongoDB * add test login with provider despite invalid authData * removed fit * fixed ignoring authData in postgres * Fix postgres test * Throw error instead of ignore * improve tests * Add mongo test * allow authData when not user class * fix tests * more tests * add condition to synthesize authData field only in _User class it is forbidden to add a custom field name beginning with `_`, so if the object is not `_User` , the transform should throw * add warning log when ignoring invalid `authData` in `_User` * add test to throw when custom field begins with underscore

mtrezza added 2 commits October 23, 2019 03:59

added ignore authData field

60c8c07

add fix for Postgres

8c1be1f

mtrezza mentioned this pull request Oct 23, 2019

Cannot read property 'facebook' of null #6140

Closed

add test for mongoDB

21a15e3

add test login with provider despite invalid authData

aeeefbf

removed fit

bc6e9bb

dplewis reviewed Oct 23, 2019

View reviewed changes

spec/ParseUser.spec.js Outdated Show resolved Hide resolved

fixed ignoring authData in postgres

a89c104

dplewis closed this Oct 23, 2019

dplewis reopened this Oct 23, 2019

Fix postgres test

2c0fc76

dplewis requested a review from acinader October 27, 2019 02:58

Throw error instead of ignore

eb7c609

dplewis changed the title ~~added ignore authData field~~ Throw error when setting authData to null Oct 27, 2019

improve tests

4a07a73

dplewis added 4 commits October 27, 2019 17:26

Add mongo test

ae3c09a

allow authData when not user class

cf70ea8

fix tests

e87c5c0

more tests

90030b6

mtrezza added 3 commits October 28, 2019 01:26

add condition to synthesize authData field only in _User class

eaa7bb9

it is forbidden to add a custom field name beginning with `_`, so if the object is not `_User` , the transform should throw

add warning log when ignoring invalid authData in _User

47fd9df

add test to throw when custom field begins with underscore

a02fcd2

acinader reviewed Oct 28, 2019

View reviewed changes

src/Adapters/Storage/Mongo/MongoTransform.js Show resolved Hide resolved

mtrezza commented Oct 28, 2019

View reviewed changes

src/Adapters/Storage/Mongo/MongoTransform.js Show resolved Hide resolved

dplewis approved these changes Oct 28, 2019

View reviewed changes

dplewis merged commit 9d781c4 into parse-community:master Oct 28, 2019

mtrezza deleted the fix-crash-on-invalid-authData branch October 28, 2019 01:46

mtrezza mentioned this pull request Dec 26, 2021

User field authData cannot be deleted parse-community/parse-dashboard#1957

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw error when setting authData to null #6154

Throw error when setting authData to null #6154

mtrezza commented Oct 23, 2019

dplewis commented Oct 23, 2019

codecov bot commented Oct 23, 2019 •

edited

Loading

mtrezza commented Oct 23, 2019 •

edited

Loading

dplewis commented Oct 23, 2019

mtrezza commented Oct 23, 2019

mtrezza commented Oct 23, 2019

dplewis commented Oct 23, 2019

dplewis commented Oct 27, 2019

mtrezza commented Oct 27, 2019

dplewis commented Oct 27, 2019

mtrezza commented Oct 27, 2019 •

edited

Loading

dplewis commented Oct 27, 2019 •

edited

Loading

mtrezza commented Oct 27, 2019 •

edited

Loading

dplewis commented Oct 27, 2019 •

edited

Loading

mtrezza commented Oct 27, 2019 •

edited

Loading

dplewis commented Oct 27, 2019

mtrezza commented Oct 28, 2019

acinader left a comment

mtrezza left a comment •

edited

Loading

dplewis commented Oct 28, 2019

mtrezza commented Oct 28, 2019

Throw error when setting authData to null #6154

Throw error when setting authData to null #6154

Conversation

mtrezza commented Oct 23, 2019

dplewis commented Oct 23, 2019

codecov bot commented Oct 23, 2019 • edited Loading

Codecov Report

mtrezza commented Oct 23, 2019 • edited Loading

dplewis commented Oct 23, 2019

mtrezza commented Oct 23, 2019

mtrezza commented Oct 23, 2019

dplewis commented Oct 23, 2019

dplewis commented Oct 27, 2019

mtrezza commented Oct 27, 2019

dplewis commented Oct 27, 2019

mtrezza commented Oct 27, 2019 • edited Loading

dplewis commented Oct 27, 2019 • edited Loading

mtrezza commented Oct 27, 2019 • edited Loading

dplewis commented Oct 27, 2019 • edited Loading

mtrezza commented Oct 27, 2019 • edited Loading

dplewis commented Oct 27, 2019

mtrezza commented Oct 28, 2019

acinader left a comment

Choose a reason for hiding this comment

mtrezza left a comment • edited Loading

Choose a reason for hiding this comment

dplewis commented Oct 28, 2019

mtrezza commented Oct 28, 2019

codecov bot commented Oct 23, 2019 •

edited

Loading

mtrezza commented Oct 23, 2019 •

edited

Loading

mtrezza commented Oct 27, 2019 •

edited

Loading

dplewis commented Oct 27, 2019 •

edited

Loading

mtrezza commented Oct 27, 2019 •

edited

Loading

dplewis commented Oct 27, 2019 •

edited

Loading

mtrezza commented Oct 27, 2019 •

edited

Loading

mtrezza left a comment •

edited

Loading