search

ether / ep_comments_page

Comments in Etherpad - No Longer Highly Experimental, now highly awesome!
Apache License 2.0
42 stars 49 forks source link

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

Open tiblu opened 4 years ago

tiblu commented 4 years ago

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 commented 4 years ago

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

tiblu commented 4 years ago

@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 commented 4 years ago

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

tiblu commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

@tiblu what's your status? :1st_place_medal:

tiblu commented 3 years ago

@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 commented 3 years ago

@tiblu no problem man, I totally know how that goes :+1:

JohnMcLear commented 3 years ago

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

rhansen commented 3 years ago

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

rhansen commented 3 years ago

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:

There are some drawbacks with the above permission model:

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 commented 3 years ago

cc @tiblu for for thoughts :)

ilmartyrk commented 3 years ago

@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 commented 3 years ago

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 (https://github.com/ether/ep_comments/issues/125#issuecomment-719514772).

And thanks to all for the input!

woeterman94 commented 3 years ago

@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 commented 3 years ago

@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:

In context of EP, either of these would work.

woeterman94 commented 3 years ago
  • 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 :

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.