diff --git a/app/assets/javascripts/discourse/app/components/group-membership-button.js b/app/assets/javascripts/discourse/app/components/group-membership-button.js index d5481e2c619..4c8239aedca 100644 --- a/app/assets/javascripts/discourse/app/components/group-membership-button.js +++ b/app/assets/javascripts/discourse/app/components/group-membership-button.js @@ -37,7 +37,7 @@ export default Component.extend({ removeFromGroup() { const model = this.model; model - .removeMember(this.currentUser) + .leave() .then(() => { model.set("is_group_user", false); this.appEvents.trigger("group:leave", model); diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index b65f29e61bb..75d91ea0c9d 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/app/assets/javascripts/discourse/app/models/group.js @@ -114,6 +114,12 @@ const Group = RestModel.extend({ }).then(() => this.findMembers(params, true)); }, + leave() { + return ajax(`/groups/${this.id}/leave.json`, { + type: "DELETE", + }).then(() => this.findMembers({}, true)); + }, + addMembers(usernames, filter, notifyUsers, emails = []) { return ajax(`/groups/${this.id}/members.json`, { type: "PUT", diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index fd09ecba2e5..440deb4894d 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -464,7 +464,7 @@ class GroupsController < ApplicationController def remove_member group = Group.find_by(id: params[:id]) raise Discourse::NotFound unless group - group.public_exit ? ensure_logged_in : guardian.ensure_can_edit!(group) + guardian.ensure_can_edit!(group) # Maintain backwards compatibility params[:usernames] = params[:username] if params[:username].present? @@ -475,16 +475,6 @@ class GroupsController < ApplicationController 'user_ids or usernames or user_emails must be present' ) if users.empty? - if group.public_exit - if !guardian.can_log_group_changes?(group) && current_user != users.first - raise Discourse::InvalidAccess - end - - unless current_user.staff? - RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed! - end - end - removed_users = [] skipped_users = [] @@ -507,6 +497,21 @@ class GroupsController < ApplicationController ) end + def leave + ensure_logged_in + unless current_user.staff? + RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed! + end + + group = Group.find_by(id: params[:id]) + raise Discourse::NotFound unless group + raise Discourse::InvalidAccess unless group.public_exit + + if group.remove(current_user) + GroupActionLogger.new(current_user, group).log_remove_user_from_group(current_user) + end + end + MAX_NOTIFIED_OWNERS ||= 20 def request_membership diff --git a/config/routes.rb b/config/routes.rb index 97c5b303511..6c111ec05f0 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -604,6 +604,7 @@ Discourse::Application.routes.draw do put "members" => "groups#add_members" put "join" => "groups#join" delete "members" => "groups#remove_member" + delete "leave" => "groups#leave" post "request_membership" => "groups#request_membership" put "handle_membership_request" => "groups#handle_membership_request" post "notifications" => "groups#set_notifications" diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 5e890d608b8..9d6236580b7 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -1234,11 +1234,11 @@ describe GroupsController do describe "membership edits" do fab!(:admin) { Fabricate(:admin) } - before do - sign_in(admin) - end - context '#add_members' do + before do + sign_in(admin) + end + it "can make incremental adds" do user2 = Fabricate(:user) @@ -1562,6 +1562,10 @@ describe GroupsController do end context '#remove_member' do + before do + sign_in(admin) + end + it "cannot remove members from automatic groups" do group.update!(automatic: true) @@ -1641,17 +1645,6 @@ describe GroupsController do end end - it 'should allow a user to leave a group' do - sign_in(other_user) - - expect do - delete "/groups/#{group.id}/members.json", - params: { username: other_user.username } - end.to change { group.users.count }.by(-1) - - expect(response.status).to eq(200) - end - it 'should not allow a underprivileged user to leave a group for another user' do sign_in(user) @@ -1716,6 +1709,57 @@ describe GroupsController do end end end + + context '#leave' do + let(:group_with_public_exit) { Fabricate(:group, public_exit: true, users: [user]) } + + it 'should allow a user to leave a group with public exit' do + sign_in(user) + + expect do + delete "/groups/#{group_with_public_exit.id}/leave.json" + end.to change { group_with_public_exit.users.count }.by(-1) + + expect(response.status).to eq(204) + end + + it 'should not allow a user to leave a group without public exit' do + sign_in(user) + + expect do + delete "/groups/#{group.id}/leave.json" + end.not_to change { group.users.count } + + expect(response).to be_forbidden + end + + it 'should not allow an anonymous user to call the leave method' do + expect do + delete "/groups/#{group_with_public_exit.id}/leave.json" + end.not_to change { group_with_public_exit.users.count } + + expect(response).to be_forbidden + end + + it 'the leave method is idempotent' do + sign_in(user) + + expect do + delete "/groups/#{group_with_public_exit.id}/leave.json" + end.to change { group_with_public_exit.users.count }.by(-1) + expect(response.status).to eq(204) + + expect do + delete "/groups/#{group_with_public_exit.id}/leave.json" + end.not_to change { group_with_public_exit.users.count } + expect(response.status).to eq(204) + + expect do + delete "/groups/#{group_with_public_exit.id}/leave.json" + end.not_to change { group_with_public_exit.users.count } + expect(response.status).to eq(204) + end + end end describe "#handle_membership_request" do