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 46329bba2d3..d5481e2c619 100644 --- a/app/assets/javascripts/discourse/app/components/group-membership-button.js +++ b/app/assets/javascripts/discourse/app/components/group-membership-button.js @@ -50,13 +50,13 @@ export default Component.extend({ joinGroup() { if (this.currentUser) { this.set("updatingMembership", true); - const model = this.model; + const group = this.model; - model - .addMembers(this.currentUser.get("username")) + group + .join() .then(() => { - model.set("is_group_user", true); - this.appEvents.trigger("group:join", model); + group.set("is_group_user", true); + this.appEvents.trigger("group:join", group); }) .catch(popupAjaxError) .finally(() => { diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index 5fcbf4618bc..6ce97c898c9 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/app/assets/javascripts/discourse/app/models/group.js @@ -127,6 +127,14 @@ const Group = RestModel.extend({ }); }, + join() { + return ajax(`/groups/${this.id}/join.json`, { + type: "PUT", + }).then(() => { + this.findMembers(); + }); + }, + addOwners(usernames, filter, notifyUsers) { return ajax(`/admin/groups/${this.id}/owners.json`, { type: "PUT", diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index ec07ac68e02..4db8e7b9210 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -333,19 +333,9 @@ class GroupsController < ApplicationController def add_members group = Group.find(params[:id]) - group.public_admission ? ensure_logged_in : guardian.ensure_can_edit!(group) + guardian.ensure_can_edit!(group) + users = users_from_params.to_a - - if group.public_admission - 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 - emails = [] if params[:emails] params[:emails].split(",").each do |email| @@ -376,17 +366,10 @@ class GroupsController < ApplicationController count: usernames_already_in_group.size )) else + notify = params[:notify_users]&.to_s == "true" uniq_users = users.uniq uniq_users.each do |user| - begin - group.add(user) - GroupActionLogger.new(current_user, group).log_add_user_to_group(user) - group.notify_added_to_group(user) if params[:notify_users]&.to_s == "true" - rescue ActiveRecord::RecordNotUnique - # Under concurrency, we might attempt to insert two records quickly and hit a DB - # constraint. In this case we can safely ignore the error and act as if the user - # was added to the group. - end + add_user_to_group(group, user, notify) end emails.each do |email| @@ -408,6 +391,19 @@ class GroupsController < ApplicationController end end + def join + ensure_logged_in + unless current_user.staff? + RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed! + end + + group = Group.find(params[:id]) + raise Discourse::InvalidAccess unless group.public_admission + + return if group.users.exists?(id: current_user.id) + add_user_to_group(group, current_user) + end + def handle_membership_request group = Group.find_by(id: params[:id]) raise Discourse::InvalidParameters.new(:id) if group.blank? @@ -650,6 +646,16 @@ class GroupsController < ApplicationController private + def add_user_to_group(group, user, notify = false) + group.add(user) + GroupActionLogger.new(current_user, group).log_add_user_to_group(user) + group.notify_added_to_group(user) if notify + rescue ActiveRecord::RecordNotUnique + # Under concurrency, we might attempt to insert two records quickly and hit a DB + # constraint. In this case we can safely ignore the error and act as if the user + # was added to the group. + end + def group_params(automatic: false) permitted_params = if automatic diff --git a/config/routes.rb b/config/routes.rb index 820509be705..97c5b303511 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -602,6 +602,7 @@ Discourse::Application.routes.draw do get "permissions" => "groups#permissions" put "members" => "groups#add_members" + put "join" => "groups#join" delete "members" => "groups#remove_member" post "request_membership" => "groups#request_membership" put "handle_membership_request" => "groups#handle_membership_request" diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index b0e31de1d8b..5e890d608b8 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -1507,26 +1507,57 @@ describe GroupsController do expect(group_history.target_user).to eq(other_user) end end + end + end - it 'should allow a user to join the group' do - sign_in(other_user) + context '#join' do + let(:public_group) { Fabricate(:public_group) } - expect do - put "/groups/#{group.id}/members.json", - params: { usernames: other_user.username } - end.to change { group.users.count }.by(1) + it 'should allow a user to join a public group' do + sign_in(user) - expect(response.status).to eq(200) - end + expect do + put "/groups/#{public_group.id}/join.json" + end.to change { public_group.users.count }.by(1) - it 'should not allow an underprivileged user to add another user to a group' do - sign_in(user) + expect(response.status).to eq(204) + end - put "/groups/#{group.id}/members.json", - params: { usernames: other_user.username } + it 'should not allow a user to join a nonpublic group' do + sign_in(user) - expect(response).to be_forbidden - end + expect do + put "/groups/#{group.id}/join.json" + end.not_to change { group.users.count } + + expect(response).to be_forbidden + end + + it 'should not allow an anonymous user to call the join method' do + expect do + put "/groups/#{group.id}/join.json" + end.not_to change { group.users.count } + + expect(response).to be_forbidden + end + + it 'the join method is idempotent' do + sign_in(user) + + expect do + put "/groups/#{public_group.id}/join.json" + end.to change { public_group.users.count }.by(1) + expect(response.status).to eq(204) + + expect do + put "/groups/#{public_group.id}/join.json" + end.not_to change { public_group.users.count } + expect(response.status).to eq(204) + + expect do + put "/groups/#{public_group.id}/join.json" + end.not_to change { public_group.users.count } + expect(response.status).to eq(204) end end