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

QUESTION/BUG: Edit comment/reply - no authorization, anyone can edit anyones content #125

Open
tiblu opened this issue May 27, 2020 · 20 comments
Assignees
Labels

Comments

@tiblu
Copy link
Contributor

tiblu commented May 27, 2020

Question

Do I understand correctly that there is no authorization for editing comment/reply? That is, anyone can edit anyones comment/reply?
https://github.com/ether/ep_comments/blob/master/commentManager.js#L272

IF that is true, any suggestions how to approach this, we'd be interested in only original author being able to edit comment/reply.

We MAY PR this.

Related to:

@JohnMcLear
Copy link
Member

If it's a socket msg the Auth will be same as the Auth on pad socket MSG's. In theory....

@tiblu
Copy link
Contributor Author

tiblu commented May 27, 2020

@JohnMcLear IF that is the only layer, then this authorization says "you can edit this pad". that also means you can edit anyones comment/reply, it does not verify the comment/reply you are editing is created by you, thus are authorized to edit.
It is fair from a user to expect that only author of the comment/reply can edit it, specially when the original authors name is persisted when the contents is edited by someone else.

@JohnMcLear
Copy link
Member

JohnMcLear commented May 27, 2020

Yeah I'd guess that's the case then. Welcome to see this change with tests!

@tiblu
Copy link
Contributor Author

tiblu commented May 27, 2020

Thanks for the info. Any quick ideas how you would approach?
IF not, we'll probably dig in at some point and hope it suits.

@JohnMcLear
Copy link
Member

Check comment.authorID matches authorID of the socket trying to make the change. Shouldn't be too hard to accomplish.

I'm sure there will be gotchas tho ;)

@JohnMcLear
Copy link
Member

@tiblu what's your status? 🥇

@tiblu
Copy link
Contributor Author

tiblu commented Oct 15, 2020

@tiblu what's your status? 🥇

Unfortunately no change and no ETA. I haven't done any software development for a while and not sure when I'm back at it. Sorry, thats the way it is.

@JohnMcLear
Copy link
Member

@tiblu no problem man, I totally know how that goes 👍

@JohnMcLear
Copy link
Member

FWIW @rhansen has fixed this now afaik, or at least some progress has been made, I will let him update :)

@rhansen
Copy link
Member

rhansen commented Oct 29, 2020

Nope, not fixed yet—I've been distracted doing other stuff. I hope to work on this next week.

@rhansen rhansen self-assigned this Oct 29, 2020
@rhansen
Copy link
Member

rhansen commented Oct 29, 2020

There are a few different ways we could address this, so I would like to get some input from everyone.

This is the behavior I would prefer to implement because it's the easiest:

  • If the user has a read-only view of the pad, that user cannot add or edit any comments (not even their own).
  • Users that can modify the pad can edit their own comments but not others.

There are some drawbacks with the above permission model:

  • Some users might want to be able to edit other people's comments.
  • Some users might want to be able to comment on a read-only pad.
  • If a user deletes their token cookie (or accesses the pad from another browser) they will be unable to edit their own comments (assuming the user is not accessing the pad via a session).

With strategically placed hooks, people should be able to write plugins that address the above issues. But until those plugins are written, some users will consider the comments plugin to be broken for their use case.

Thoughts?

@JohnMcLear
Copy link
Member

cc @tiblu for for thoughts :)

@ilmartyrk
Copy link
Collaborator

@rhansen @JohnMcLear @tiblu this is how I solved it, sorry for no pull request, haven' t had the time to do it as our fork has other changes too https://github.com/citizenos/ep_comments/commit/d5c1bf1c80f81812656940e8e3b696ad879684f3

@tiblu
Copy link
Contributor Author

tiblu commented Oct 31, 2020

There are a few different ways we could address this, so I would like to get some input from everyone.

This is the behavior I would prefer to implement because it's the easiest:

  • If the user has a read-only view of the pad, that user cannot add or edit any comments (not even their own).
  • Users that can modify the pad can edit their own comments but not others.

I think this behavior would work for us (Citizen OS).
Not sure about other EP users, but it seems an interesting use-case to enable changing other peoples comments.

Also, in @ilmartyrk I trust (#125 (comment)).

And thanks to all for the input!

@woeterman94
Copy link
Contributor

@rhansen @JohnMcLear @tiblu this is how I solved it, sorry for no pull request, haven' t had the time to do it as our fork has other changes too citizenos@d5c1bf1

To be honest I would also hide the edit and delete button when the user has no access.

@tiblu
Copy link
Contributor Author

tiblu commented Nov 12, 2020

@rhansen @JohnMcLear @tiblu this is how I solved it, sorry for no pull request, haven' t had the time to do it as our fork has other changes too citizenos@d5c1bf1

To be honest I would also hide the edit and delete button when the user has no access.

@woeterman94 Few approaches come to mind:

  • Show functionality, and show error, when a user tries to perform action (current solution).
  • Hide functionality that user has no permission to use.
  • Show functionality, that user has no permission to use, but disable it and show tooltip with info WHY the functionality is disabled.

In context of EP, either of these would work.

@woeterman94
Copy link
Contributor

  • Hide functionality that user has no permission to use.

I would go for option 2, but perform a check in the backend.
Showing an error when someone clicks edit or delete isn't really user-friendly. The button shouldn't be there in the first place.

@Chostakovitch
Copy link

Hello everyone and @ilmartyrk,

As @rhansen mentionned :

Some users might want to be able to edit other people's comments.

And more importantly :

If a user deletes their token cookie (or accesses the pad from another browser) they will be unable to edit their own comments

Since #163, users have reported that they cannot remove or edit other's comments, which is part of their usual workflow.

Would it be possible to use a configuration parameter to enable or disable this behaviour ? It don't see a "good reason" to disable editing/remove other's comment in all situations.

Unfortunately, I don't really have time nor skills to code that, so I would totally understand if you do not have time to "fix" it.

@woeterman94
Copy link
Contributor

woeterman94 commented Nov 24, 2020

If a user deletes their token cookie (or accesses the pad from another browser) they will be unable to edit their own comments

A comment and a session are both linked to an authorId right?

Some users might want to be able to edit other people's comments.

Don't know. I think by default it shouldn't be allowed.

Since #163, users have reported that they cannot remove or edit other's comments, which is part of their usual workflow.

I'd suggest you don't upgrade and use the previous version which allows it?

@Chostakovitch
Copy link

Chostakovitch commented Nov 24, 2020

@woeterman94

A comment and a session are both linked to an authorId right?

I think so, the point is that if you delete your cookies for whatever reason or use a different computer/browser, the comments have no way to get deleted, and some users will periodically purge cookies from their browser.

Don't know. I think by default it shouldn't be allowed.

I agree. The problem is more the sudden behaviour switch. Our instance being active for more than 2 years and used mostly by project groups and associations, the workflow with comments is often :

  • Someone creates a pad and is the main author, asks for comments from others
  • Others will comment, suggest, etc
  • When the comment is resolved, it is deleted for clarity; indeed a document with too much comments is hard to read.

I'd suggest you don't upgrade and use the previous version which allows it?

As a temporary fix, yes, but next versions will get bugfixes, etc.

I apologize if I sounded disrespectful in any way. My point is just that being able to edit and remove other's comment is a realistic and common features for a lot of workflows, and that it should be configurable with a plugin parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants