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

[FIX] /groups.invite not allow a user to invite even with permission #11010

Merged
merged 7 commits into from Jun 15, 2018

Conversation

Projects
4 participants
@MarcosSpessatto
Copy link
Member

commented Jun 5, 2018

Closes #10639

@MarcosSpessatto MarcosSpessatto added this to the 0.66.0 milestone Jun 5, 2018

@MarcosSpessatto MarcosSpessatto self-assigned this Jun 5, 2018

@MarcosSpessatto MarcosSpessatto added this to Desireable in June/2018 via automation Jun 5, 2018

@MarcosSpessatto MarcosSpessatto changed the title [FIX] Fix REST groups invite, that did not allow a user to invite even with permission [FIX] REST groups invite, that did not allow a user to invite even with permission Jun 5, 2018

@MarcosSpessatto MarcosSpessatto moved this from Desireable to Review/QA in June/2018 Jun 5, 2018

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11010 Jun 5, 2018 Inactive

@ggazzo ggazzo requested a review from Hudell Jun 11, 2018

const params = this.requestParams();
if (canAddUserToAnyPrivateGroup) {
if (params.roomId && params.roomId.trim()) {
findResult = RocketChat.models.Subscriptions.findByRoomId(params.roomId).fetch()[0];

This comment has been minimized.

Copy link
@Hudell

Hudell Jun 12, 2018

Member

Why not find the room directly? Looking for subscriptions will cause the code to fail on empty rooms.

@@ -54,6 +54,14 @@ class ModelSubscriptions extends RocketChat.models._Base {
return this.findOne(query);
}

findOneByRoomName(roomName) {

This comment has been minimized.

Copy link
@Hudell

Hudell Jun 12, 2018

Member

If you use the room directly, this method won't be necessary.

@ggazzo
Copy link
Member

left a comment

I think the problem is here findPrivateGroupByIdOrName. if i'm not mistaken, we have the same problem for more endpoints, please check the functions that use findPrivateGroupByIdOrName and let me know if I made some confusion, but I think all these methods just works if you are joined in the room(even if you are an admin)...

@Hudell

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

@ggazzo Those methods don't work if you've not joined the room.

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11010 Jun 13, 2018 Inactive

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11010 Jun 13, 2018 Inactive


const user = this.getUserFromParams();

Meteor.runAsUser(this.userId, () => {
Meteor.call('addUserToRoom', { rid: findResult.rid, username: user.username });
Meteor.call('addUserToRoom', { rid: findResult._id, username: user.username });

This comment was marked as resolved.

Copy link
@Hudell

Hudell Jun 13, 2018

Member

This gets the id of the object, which is valid when it's a room, but when the user doesn't have the 'add-user-to-any-p-room', the findResult object is still going to be a subscription.

It's better to assign the id to a variable inside the conditional (room._id when the user has the permission, or subscription.rid when the user doesn't have it) and then use the variable here.

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11010 Jun 13, 2018 Inactive

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11010 Jun 13, 2018 Inactive

@Hudell

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

The current solution works perfectly. Can you check if there are more endpoints using the findPrivateGroupByIdOrName method, where the same problem may be happening?

@ggazzo

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

I think for this case you should just call the dpp method

@ggazzo ggazzo moved this from Review/QA to In progress in June/2018 Jun 13, 2018

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11010 Jun 14, 2018 Inactive

@Hudell Hudell dismissed their stale review Jun 14, 2018

Changes were made

@MarcosSpessatto MarcosSpessatto moved this from In progress to Review/QA in June/2018 Jun 14, 2018

@ggazzo ggazzo temporarily deployed to rocket-chat-pr-11010 Jun 15, 2018 Inactive

@ggazzo

ggazzo approved these changes Jun 15, 2018

@ggazzo ggazzo changed the title [FIX] REST groups invite, that did not allow a user to invite even with permission [FIX] /groups.invite not allow a user to invite even with permission Jun 15, 2018

@ggazzo ggazzo merged commit 85b662b into develop Jun 15, 2018

4 of 5 checks passed

ci/circleci: build-and-test/hold Your job is on hold on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-with-oplog Your tests passed on CircleCI!
Details
ci/circleci: test-without-oplog Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

June/2018 automation moved this from Review/QA to Closed Jun 15, 2018

@ggazzo ggazzo deleted the fix-rest-groups-invite branch Jun 15, 2018

@rodrigok rodrigok referenced this pull request Jun 28, 2018

Merged

Release 0.66.0 #11278

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.