search

Mottokrosh / Sheet

A cloud based character sheet for the Pathfinder Roleplaying Game
MIT License
47 stars 32 forks source link

Adding separate endpoint for explicitly patching comments, disallowin… #74

Closed edreeseg closed 2 years ago

edreeseg commented 3 years ago

…g any other character modification unless user is authorized as creator of said character.

We make use of the creator information passed to use in req.body, and compare it to the information stored in the sheetuser cookie. If they don't match, we reject any attempts to edit with a 403.

We don't want to restrict the GET endpoint, as this is also used for sharing of a character's information. This means that the user will still be able to open a character's information in editable sheet form, but any changes made will throw a 403 and cause a redirect back to login.

Because comments are also done by editing the character, I've added a PATCH endpoint to specifically update the character's comments. This explicitly extracts the comments key from req.body, and only modifies that single key. This endpoint does not require that the user adding or deleting the comments be the same as the creator. This is perhaps not ideal, but since we haven't had a concept of comments being linked with specific accounts, it isn't really a regression.

What this amounts to is - a user can still open an edit sheet that is not their own. However, making edits to this sheet is not allowed unless they are the creator of the character. This also applies to archiving and deleting of a character, as all of those actually go through that same PUT endpoint.

I did have a question, as I want to make sure I'm visualizing things properly. Right now we've got a /collections/characters/:id DELETE endpoint that I don't see being used anywhere. Do we have plans for that? I'd initially thought that's where character deletion is handled.

Thanks so much once more for taking the time to review this. This should, at least in some way, address #24 .

edreeseg commented 3 years ago

Moving forward I'd ideally like to remove all information but the user's name from the user key sent to the front-end. Rather than checking the user from req.body, we'll instead grab the character with .findOne , get the user ID from that information, and then make the diff based on that. No need to send more information about the creator than is necessary, if we aren't sure whether a GET response is going to the creator or to someone else.

Mottokrosh commented 3 years ago

Thanks for this! The code looks sound, though I haven't had the chance to test it yet myself.

I also completely missed the PR, my apologies. Feel free to tag me directly as reviewer in future, as there are not so many active developers working on this, as you can see. 😄

I'm unsure about the DELETE endpoint. I thought it was used to delete archived characters, but I could be wrong.