Vulcan-Starter
Vulcan-Starter copied to clipboard
VulcanJS
Vote resolver allows for multiple votes if you submit too many requests in a short time
The current vote resolver checks the database whether you've already voted on something, and if you have, cancels your vote, which changes the score of the item you are voting on. If you vote again before the database operation completes, then the resolver is going to cancel your vote twice, which means that the score decreases by twice your voting power. This allows you to arbitrarily manipulate the scores of various items. This seems quite bad and we probably want to fix that.
Can you specify how to recreate the bug? Is it just by clicking the upvote button rapidly?
There's definitely something going on with the vote module but I'm not seeing the behaviour as described in this issue. When I take a fresh install of example-forum, create an account, and upvote a post, even though the code seems to indicate that my vote should be canceled if I try to upvote a post, the application appears to just count upvotes indefinitely:
https://cl.ly/7155215d0564
I've traced out the code and I can't determine exactly where or how this is happening due (I assume due to my unfamiliarity with graphQL) but what I think I am seeing is on the second click of the upvote button, currentUserVotes is being correctly emptied, but the mutation to add a vote is still running which causes the client data to be overwritten by new props passed down.
We fixed a bunch of bugs related to this in our codebase. You can take a look at our Vulcan fork to figure out the differences in implementation: https://github.com/LessWrong2/Vulcan
On Fri, Oct 19, 2018 at 11:33 AM James Wrubel [email protected] wrote:
There's definitely something going on with the vote module but I'm not seeing the behaviour as described in this issue. When I take a fresh install of example-forum, create an account, and upvote a post, even though the code seems to indicate that my vote should be canceled if I try to upvote a post, the application appears to just count upvotes indefinitely:
https://cl.ly/7155215d0564
I've traced out the code and I can't determine exactly where or how this is happening due (I assume due to my unfamiliarity with graphQL) but what I think I am seeing is on the second click of the upvote button, currentUserVotes https://github.com/VulcanJS/Vulcan/blob/devel/packages/vulcan-voting/lib/modules/vote.js#L140 is being correctly emptied, but the mutation to add a vote is still running which causes the client data to be overwritten by new props passed down.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/VulcanJS/Vulcan-Starter/issues/31#issuecomment-431455062, or mute the thread https://github.com/notifications/unsubscribe-auth/AIq25fFXT4txO7hZvjNUSRB1ULRgE58Dks5umhk6gaJpZM4SSbdV .
@Discordius could you submit a PR back to devel if you think those fixes could be helpful?
Likely related to this: https://vulcanjs.slack.com/archives/C02L990AF/p1538612081000100
In particular, vote.js has a call to:
await Connectors.delete(Votes, {documentId: document._id, userId: user._id});But that breaks now, because that gets translated to{ userId: <userId>, _id: <documentId> }Which is importantly different, and basically breaks all the clearVote functionality
@Discordius I was able to extract what I needed from your repo and I got voting working correctly - thank you! It was quite a lot of changes and they're in the voting package, not Vulcan-Starter. Still I have what I changed in a single commit so I'm happy to add it as a PR, even if it's just for someone to take a look at and re-implement properly.
Good idea - there's no reason not to make it public so I just opened it. I had originally started trying to keep individual commits but that got too hard, so these four commits should be enough to correct voting functionality: 1 2 3 4 . That last one is the big one. To be honest I haven't really traced through all of these changes to understand how they work; It is definitely possible some of these are not needed. But I can confirm these worked in my implementation. I'm happy to try and assist if you run in to issues implementing these changes, to the extent I'm able.
Quote from Halima Olapade on Slack that might explain this issue: "I think i figured out why the inbuilt voting is breaking. It's cause the Vote collection schema has a field called "documentId" but "documentId" seems to be some kind of reserved identifier. As a result this query always fails: https://github.com/VulcanJS/Vulcan/blob/devel/packages/vulcan-voting/lib/modules/vote.js#L52 so the system can't tell that there's a previous upvote that should be annulled""
