Fix: properly pass req.user to liveQuery triggers by dblythy · Pull Request #7296 · parse-community/parse-server

dblythy · 2021-03-24T17:07:59Z

New Pull Request Checklist

I am not disclosing a vulnerability.
I am creating this PR in reference to an issue.

Issue Description

When I replaced the LQ triggers auth lookup with getAuthForSessionToken, I forgot to destructure auth. My bad.

This results in req.user always being undefined for LQ triggers. This only affects the master branch.

First commit is failing test.

Closes: #7318

dplewis · 2021-03-24T17:15:08Z

Can you write a test case? Looks good

codecov · 2021-03-24T18:50:21Z

Codecov Report

Merging #7296 (4065882) into master (3638b0e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #7296   +/-   ##
=======================================
  Coverage   93.91%   93.92%           
=======================================
  Files         181      181           
  Lines       13195    13210   +15     
=======================================
+ Hits        12392    12407   +15     
  Misses        803      803

Impacted Files	Coverage Δ
src/LiveQuery/ParseLiveQueryServer.js	`95.50% <100.00%> (+0.17%)`	⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`95.36% <0.00%> (-0.16%)`	⬇️
src/Adapters/Files/GridFSBucketAdapter.js	`80.32% <0.00%> (+0.81%)`	⬆️
src/ParseServerRESTController.js	`98.50% <0.00%> (+1.49%)`	⬆️

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 3638b0e...4065882. Read the comment docs.

dblythy · 2021-03-24T18:59:05Z

Tests for this seem to be very flaky. Not sure why. Sometimes sesssionToken is passed, sometimes it's not. I think it's possibly because client is cached across request by the JS SDK. Any pointers appreciated!

Test also work with fit but not without it.

dblythy · 2021-03-26T08:17:14Z

@mtrezza @davimacedo just noting that this will have to be solved before next release otherwise LQ triggers won’t have .user available to them

mtrezza · 2021-04-04T08:52:35Z

Test also work with fit but not without it.

This is usually an indication that the test is influenced by other tests that run previously. It may have to do with the recent restructuring of tests. It may also be that your added tests influence each other when not run with fit.

mtrezza · 2021-04-04T09:00:40Z

~~I classified this as a bug with severity S2, because there is no known workaround.~~

I just noticed that this is a PR and it doesn't seem to have a referenced open issue.

@dblythy How is this PR and your finding related to the issue #7295, which has been closed by the author who wrote that the issue was just a misconfiguration. If this is unrelated to the issue, can you please open a new one and correct the reference?

dblythy · 2021-04-04T10:57:14Z

I discovered this bug during the investigation for #7295, and I assumed that this was the bug that @yanuarizalk had found. It appears now that it wasn't. I've reopened an issue.

ghost · 2021-04-04T11:03:48Z

	Warnings
⚠️	Please add a changelog entry for your changes.

Generated by 🚫 dangerJS

dblythy · 2021-04-04T11:04:50Z

New Issue: #7318

mtrezza · 2021-04-04T11:07:19Z

Thanks, so we can apply the bug label and remind us to address this before new release.

dblythy · 2021-04-04T11:51:53Z

Should be good to go. Do you want me to add a changelog? This bug was introduced in the master and fixed in the master so I don't see the purpose of the changelog imo.

mtrezza · 2021-04-04T12:30:49Z

Yes, I also think no changelog entry is necessary.

Are the tests still flaky?

dblythy · 2021-04-04T12:35:44Z

Are the tests sill flaky?

Nope! The issue was using client.sessionToken over client.getSubscriptionInfo(requestId).sessionToken.

It seems as though client.sessionToken gets cached between tests, which can make it flaky. Using client.getSubscriptionInfo(requestId).sessionToken seems to be 100% solid as it matches requestId, not just client. I've tested this approach around 10 times and tests are consistently passing fine.

mtrezza · 2021-04-04T13:46:30Z

Could you look at the codecov? It says some added lines are not covered by tests.

dblythy · 2021-04-23T15:33:09Z

Getting an error:

Failures:
1) OAuth Should fail a GET request
  Message:
    Uncaught exception: SyntaxError: Unexpected end of JSON input
  Stack:
        at <Jasmine>
        at IncomingMessage.<anonymous> (/home/runner/work/parse-server/parse-server/lib/Adapters/Auth/OAuth1Client.js:3:462)
        at IncomingMessage.emit (events.js:327:22)
        at endReadableNT (internal/streams/readable.js:1327:12)
        at processTicksAndRejections (internal/process/task_queues.js:80:21)

Any pointers appreciated!

mtrezza · 2021-04-23T17:04:51Z

+    await object.destroy();
+    await new Promise(resolve => setTimeout(resolve, 200));
+    for (const key in calls) {
+      expect(calls[key]).toHaveBeenCalled();


mtrezza · 2021-04-23T17:18:05Z

I restarted the test to make sure it's not just flaky - it still occurs.

My guess, it's probably the refactoring of the auth object in this PR. Did you try to debug step through the test?

dblythy · 2021-04-24T00:49:40Z

Thanks @mtrezza, it appears this error is thrown on #7365 as well, so I'm assuming this error exists on the master branch. I've changed the twitter endpoint that the failing test checks for and seems to be all fine. What do you think?

mtrezza · 2021-04-24T00:52:12Z

The master branch passes currently, it could be flaky? I just restarted the jobs on master, let's see.

Edit: You were right, same error on master, even though the build passed 3 days ago. Maybe something has changed externally.

There was likely a change in the Twitter API, as you said the test checks an endpoint. It's possible that the test points to an obsolete API version and it has been force-upgraded.

I would just debug the test to get a hint why it fails. This should be fixed in a separate issue / PR though.

dblythy · 2021-04-24T01:08:42Z

The test performs a GET request on this url, which now returns an empty page. I'm assuming this is a change on Twitter's end.

The test is expecting a response of {"errors":[{"code":215,"message":"Bad Authentication data."}]}. When the path is changed to /1.1/oauth/request_token, the correct error is thrown.

Will create a new issue and PR.

mtrezza

Looks good! Thanks for fixing this.

dblythy · 2021-05-02T09:36:23Z

Thank you for the review Manuel!

parseplatformorg · 2021-11-01T16:29:54Z

🎉 This change has been released in version 5.0.0-beta.1

parseplatformorg · 2022-03-14T13:01:56Z

🎉 This change has been released in version 5.0.0

dblythy commented Mar 24, 2021

View reviewed changes

Comment thread spec/ParseLiveQuery.spec.js Outdated

dplewis approved these changes Mar 24, 2021

View reviewed changes

mtrezza added type:bug Impaired feature or lacking behavior that is likely assumed ⚡S2 labels Apr 4, 2021

mtrezza removed type:bug Impaired feature or lacking behavior that is likely assumed ⚡S2 labels Apr 4, 2021

mtrezza reviewed Apr 4, 2021

View reviewed changes

Comment thread src/LiveQuery/ParseLiveQueryServer.js Outdated

mtrezza requested changes Apr 5, 2021

View reviewed changes

Comment thread src/LiveQuery/ParseLiveQueryServer.js Outdated

Comment thread src/LiveQuery/ParseLiveQueryServer.js Outdated

mtrezza reviewed Apr 5, 2021

View reviewed changes

Comment thread src/LiveQuery/ParseLiveQueryServer.js Outdated

mtrezza reviewed Apr 5, 2021

View reviewed changes

Comment thread src/LiveQuery/ParseLiveQueryServer.js Outdated

mtrezza reviewed Apr 13, 2021

View reviewed changes

Comment thread src/LiveQuery/ParseLiveQueryServer.js

mtrezza requested changes Apr 14, 2021

View reviewed changes

Comment thread spec/ParseLiveQuery.spec.js Outdated

Comment thread spec/ParseLiveQuery.spec.js Outdated

dblythy added 4 commits April 24, 2021 01:21

fix: properly pass req.user to liveQuery triggers

2f454d0

Update ParseLiveQueryServer.js

2d8e75b

Update ParseLiveQueryServer.js

fb17e47

fix failing tests

2bd2106

dblythy added 7 commits April 24, 2021 01:21

Update ParseLiveQuery.spec.js

5781e92

Update ParseLiveQueryServer.js

0dd2c54

Update ParseLiveQueryServer.js

8b6763c

Update ParseLiveQueryServer.js

2ca9c66

Update ParseLiveQueryServer.js

e5a2f82

Update ParseLiveQueryServer.js

67a39c7

Update ParseLiveQuery.spec.js

89345ad

dblythy force-pushed the liveQueryACLFix branch from 517d050 to 89345ad Compare April 23, 2021 15:21

mtrezza reviewed Apr 23, 2021

View reviewed changes

Update OAuth1.spec.js

a4fe237

Update ParseLiveQuery.spec.js

837ff4d

dblythy mentioned this pull request Apr 24, 2021

Tests fail on master due to Twitter API #7369

Closed

6 tasks

dblythy added 2 commits April 24, 2021 11:13

Update OAuth1.spec.js

eddfcb5

Merge branch 'master' into liveQueryACLFix

4065882

dblythy requested a review from mtrezza May 2, 2021 06:50

mtrezza approved these changes May 2, 2021

View reviewed changes

mtrezza closed this May 2, 2021

mtrezza reopened this May 2, 2021

mtrezza merged commit 51e0800 into parse-community:master May 2, 2021

dblythy deleted the liveQueryACLFix branch May 2, 2021 09:36

parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021

parseplatformorg added the state:released Released as stable version label Mar 14, 2022

Uh oh!

Conversation

dblythy commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Pull Request Checklist

Issue Description

Uh oh!

dplewis commented Mar 24, 2021

Uh oh!

Uh oh!

codecov Bot commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dblythy commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblythy commented Mar 26, 2021

Uh oh!

mtrezza commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrezza commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblythy commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 4, 2021

Uh oh!

dblythy commented Apr 4, 2021

Uh oh!

mtrezza commented Apr 4, 2021

Uh oh!

dblythy commented Apr 4, 2021

Uh oh!

mtrezza commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblythy commented Apr 4, 2021

Uh oh!

mtrezza commented Apr 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dblythy commented Apr 23, 2021

Uh oh!

mtrezza Apr 23, 2021

Choose a reason for hiding this comment

Uh oh!

mtrezza commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblythy commented Apr 24, 2021

Uh oh!

mtrezza commented Apr 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblythy commented Apr 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrezza left a comment

Choose a reason for hiding this comment

Uh oh!

dblythy commented May 2, 2021

Uh oh!

parseplatformorg commented Nov 1, 2021

Uh oh!

parseplatformorg commented Mar 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

dblythy commented Mar 24, 2021 •

edited

Loading

codecov Bot commented Mar 24, 2021 •

edited

Loading

dblythy commented Mar 24, 2021 •

edited

Loading

mtrezza commented Apr 4, 2021 •

edited

Loading

mtrezza commented Apr 4, 2021 •

edited

Loading

dblythy commented Apr 4, 2021 •

edited

Loading

mtrezza commented Apr 4, 2021 •

edited

Loading

mtrezza commented Apr 23, 2021 •

edited

Loading

mtrezza commented Apr 24, 2021 •

edited

Loading

dblythy commented Apr 24, 2021 •

edited

Loading