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.

Sign up for GitHub

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

Jump to bottom

[FIX] Several problems with read-only rooms and muted users #11311

Merged
merged 27 commits into from
May 16, 2019
Merged

[FIX] Several problems with read-only rooms and muted users #11311

merged 27 commits into from
May 16, 2019

Conversation

Hudell
Copy link
Contributor

@Hudell Hudell commented Jul 2, 2018

Closes #4769

The permission was checked when adding users to readonly channel (to determine if they are muted or not) but if the user received or lost the permission later, it would never be checked again.

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11311 July 2, 2018 16:25 Inactive
@ggazzo ggazzo added this to the 0.67.0 milestone Jul 6, 2018
@theorenck theorenck modified the milestones: 0.67.0, 0.68.0 Jul 19, 2018
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11311 August 20, 2018 13:38 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11311 August 20, 2018 13:40 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11311 August 20, 2018 13:43 Inactive
@Hudell Hudell added this to the 0.69.0 milestone Aug 20, 2018
@rodrigok
Copy link
Member

@Hudell wouldn't be better to ignore the array of muted users since we are putting everyone in there?

@Hudell
Copy link
Contributor Author

Hudell commented Aug 20, 2018

Looks like there are some places where only the muted list is checked. I'll take a better look into the whole feature.

@Hudell Hudell mentioned this pull request Sep 13, 2018
Closed
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11311 October 11, 2018 05:27 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11311 October 15, 2018 18:27 Inactive
@Hudell
Copy link
Contributor Author

Hudell commented Oct 15, 2018

With the new changes, the muted list will only be used for manually muted users. When a room is set as readonly, the permission for sending messages anyway will be checked to determine if the user can send it.

I've also added a migration to clear the muted list of readonly rooms, since that list used to have every user in the room added automatically to it.

sampaiodiego
sampaiodiego previously requested changes Oct 20, 2018
View reviewed changes
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along with inline comments, this PR is removing a functionality of allowing specific people to talk on a read only room. That was done by unmuting the user from members list view:
image

packages/rocketchat-lib/client/lib/roomTypes.js Outdated Show resolved Hide resolved
packages/rocketchat-lib/server/lib/processDirectEmail.js Outdated Show resolved Hide resolved
packages/rocketchat-lib/server/methods/sendMessage.js Outdated Show resolved Hide resolved
packages/rocketchat-lib/server/methods/sendMessage.js Outdated Show resolved Hide resolved
@Hudell Hudell requested a review from sampaiodiego March 25, 2019 02:05
MarcosSpessatto
MarcosSpessatto requested changes Mar 29, 2019
View reviewed changes
Copy link
Member

@MarcosSpessatto MarcosSpessatto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should change all these imports from absolute paths(e.g from /app/models) to relative imports, mainly because @rodrigok made a PR to adopt this behavior.

app/authorization/client/hasPermission.js Outdated Show resolved Hide resolved
app/authorization/client/hasPermission.js Outdated Show resolved Hide resolved
app/authorization/client/hasPermission.js Outdated Show resolved Hide resolved
@Hudell Hudell changed the title [FIX] post-readonly permission is only checked when the user is added to the channel [FIX] Several problems with read-only rooms May 7, 2019
@Hudell Hudell changed the title [FIX] Several problems with read-only rooms [FIX] Several problems with read-only rooms and muted users May 7, 2019
@Hudell Hudell mentioned this pull request May 7, 2019
Closed
@Hudell Hudell requested a review from MarcosSpessatto May 7, 2019 19:06
@MarcosSpessatto
Copy link
Member

@Hudell can you please take a look on tests?

@rodrigok rodrigok modified the milestones: Short-term, 1.1.0 May 15, 2019
@rodrigok
Copy link
Member

@Hudell can you fix the tests?

@rodrigok rodrigok merged commit 8b81b17 into develop May 16, 2019
@rodrigok rodrigok deleted the fix.post-readonly-permission branch May 16, 2019 18:28
@sampaiodiego sampaiodiego mentioned this pull request May 28, 2019
Merged
@Hudell Hudell mentioned this pull request Aug 21, 2019
Closed
@winterstefan winterstefan mentioned this pull request Sep 16, 2019
Closed
@pierre-lehnen-rc pierre-lehnen-rc mentioned this pull request May 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcosSpessatto MarcosSpessatto MarcosSpessatto approved these changes

@sampaiodiego sampaiodiego Awaiting requested review from sampaiodiego

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

readonly and post-readonly permission not working properly
8 participants
@Hudell @rodrigok @MarcosSpessatto @sampaiodiego @engelgabriel @theorenck @ggazzo @pierre-lehnen-rc