Attached is a patch implementing atomic changes for both affiliation and
roles change requests.

I removed room_mt:set_role() because it was no longer used once I called
the newly added room_mt:set_roles() in the request handling method.

I tested the affiliation changing quite thoroughly but fell short of
that when testing role changes, since role changes were always
greyed-out in Gajim. Is there another (small) client or test utility I
could test this with?

Unfortunately the diff just does not get prettier due to more changes
that are all around the muc plugin. I hope you can decipher it anyway.

Best Regards
Lennart

-- 
You received this message because you are subscribed to the Google Groups 
"prosody-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to prosody-dev+unsubscr...@googlegroups.com.
To post to this group, send email to prosody-dev@googlegroups.com.
Visit this group at https://groups.google.com/group/prosody-dev.
For more options, visit https://groups.google.com/d/optout.
# HG changeset patch
# User Lennart Sauerbeck <de...@lennart.sauerbeck.org>
# Date 1489859248 -3600
#      Sat Mar 18 18:47:28 2017 +0100
# Node ID a33be5300a036efc2c0ca2310b44b96285b6523e
# Parent  484ea6201c6c28793c6d1a94fea5264811e12889
muc: Allow clients to change multiple affiliations or roles at once (#345)

According to XEP-0045 sections 9.2, 9.5 and 9.8 affiliation lists and role
lists should allow mass-modification. Prosody however would just use the
first entry of the list and ignore the rest. This is fixed by introducing
a `for` loop to `set` stanzas of the respective `muc#admin` namespace.

The change requested by the client will be treated as atomic: Either the
whole change can be applied, or nothing is applied. The way this is
implemented is to first check if any of the requested affiliation or role
changes would be inconsistent and only if that is not the case all of them
are applied.

The standard defines it a possible to change affiliations and roles at the
same time. As such the requested change is checked for both affiliation and
role inconistencies. For the error message affiliation changes are deemed
more important: If both affiliation and role change checks happen upon an
error, the affiliation error message will be passed to the client.

References: #345

diff -r 484ea6201c6c -r a33be5300a03 plugins/muc/muc.lib.lua
--- a/plugins/muc/muc.lib.lua	Thu Apr 27 12:41:53 2017 +0200
+++ b/plugins/muc/muc.lib.lua	Sat Mar 18 18:47:28 2017 +0100
@@ -773,10 +773,16 @@
 			local affiliation = self:get_affiliation(actor);
 			local current_nick = self._jid_nick[actor];
 			local role = current_nick and self._occupants[current_nick].role or self:get_default_role(affiliation);
-			local item = stanza.tags[1].tags[1];
-			if item and item.name == "item" then
-				if type == "set" then
-					local callback = function() origin.send(st.reply(stanza)); end
+			if type == "set" then
+				local at_least_one_item_provided = false;
+				local callback = function() origin.send(st.reply(stanza)); end
+
+				-- Gather all changes to affiliations and roles
+				local jid_affiliation = {};
+				local jidnick_role = {};
+				for item in stanza.tags[1]:childtags("item") do
+					at_least_one_item_provided = true;
+
 					if item.attr.jid then -- Validate provided JID
 						item.attr.jid = jid_prep(item.attr.jid);
 						if not item.attr.jid then
@@ -791,17 +797,43 @@
 						local nick = self._jid_nick[item.attr.jid];
 						if nick then item.attr.nick = select(3, jid_split(nick)); end
 					end
+
 					local reason = item.tags[1] and item.tags[1].name == "reason" and #item.tags[1] == 1 and item.tags[1][1];
 					if item.attr.affiliation and item.attr.jid and not item.attr.role then
-						local success, errtype, err = self:set_affiliation(actor, item.attr.jid, item.attr.affiliation, callback, reason);
-						if not success then origin.send(st.error_reply(stanza, errtype, err)); end
+						jid_affiliation[item.attr.jid] = { ["affiliation"] = item.attr.affiliation, ["reason"] = reason };
 					elseif item.attr.role and item.attr.nick and not item.attr.affiliation then
-						local success, errtype, err = self:set_role(actor, self.jid.."/"..item.attr.nick, item.attr.role, callback, reason);
-						if not success then origin.send(st.error_reply(stanza, errtype, err)); end
+						jidnick_role[item.attr.jid.."/"..item.attr.nick] = { ["role"] = item.attr.role, ["reason"] = reason };
 					else
 						origin.send(st.error_reply(stanza, "cancel", "bad-request"));
+						return;
 					end
-				elseif type == "get" then
+				end
+
+				if not at_least_one_item_provided then
+					origin.send(st.error_reply(stanza, "cancel", "bad-request"));
+					return;
+				else
+					local can_set_affiliations, errtype_aff, err_aff = self:can_set_affiliations(actor, jid_affiliation)
+					local can_set_roles, errtype_role, err_role = self:can_set_roles(actor, jidnick_role)
+
+					if can_set_affiliations and can_set_roles then
+						local success, errtype, err = self:set_affiliations(actor, jid_affiliation, callback);
+						local success, errtype, err = self:set_roles(actor, jidnick_role, callback);
+					else
+						if not can_set_affiliations then
+							origin.send(st.error_reply(stanza, ertype_aff, err_aff));
+						elseif not can_set_roles then
+							origin.send(st.error_reply(stanza, errtype_role, err_role));
+						else
+							origin.send(st.error_reply(stanza, "cancel", "bad-request"));
+						end
+
+						return;
+					end
+				end
+			elseif type == "get" then
+				local item = stanza.tags[1].tags[1];
+				if item and item.name == "item" then
 					local _aff = item.attr.affiliation;
 					local _rol = item.attr.role;
 					if _aff and not _rol then
@@ -839,9 +871,9 @@
 					else
 						origin.send(st.error_reply(stanza, "cancel", "bad-request"));
 					end
+				else
+					origin.send(st.error_reply(stanza, "cancel", "bad-request"));
 				end
-			elseif type == "set" or type == "get" then
-				origin.send(st.error_reply(stanza, "cancel", "bad-request"));
 			end
 		elseif xmlns == "http://jabber.org/protocol/muc#owner"; and (type == "get" or type == "set") and stanza.tags[1].name == "query" then
 			if self:get_affiliation(stanza.attr.from) ~= "owner" then
@@ -969,68 +1001,102 @@
 	if not result and self._affiliations[host] == "outcast" then result = "outcast"; end -- host banned
 	return result;
 end
-function room_mt:set_affiliation(actor, jid, affiliation, callback, reason)
-	jid = jid_bare(jid);
-	if affiliation == "none" then affiliation = nil; end
-	if affiliation and affiliation ~= "outcast" and affiliation ~= "owner" and affiliation ~= "admin" and affiliation ~= "member" then
-		return nil, "modify", "not-acceptable";
-	end
+function room_mt:can_set_affiliations(actor, jid_affiliation)
+	local actor_affiliation;
 	if actor ~= true then
-		local actor_affiliation = self:get_affiliation(actor);
-		local target_affiliation = self:get_affiliation(jid);
-		if target_affiliation == affiliation then -- no change, shortcut
-			if callback then callback(); end
-			return true;
-		end
-		if actor_affiliation ~= "owner" then
-			if affiliation == "owner" or affiliation == "admin" or actor_affiliation ~= "admin" or target_affiliation == "owner" or target_affiliation == "admin" then
-				return nil, "cancel", "not-allowed";
-			end
-		elseif target_affiliation == "owner" and jid_bare(actor) == jid then -- self change
-			local is_last = true;
-			for j, aff in pairs(self._affiliations) do if j ~= jid and aff == "owner" then is_last = false; break; end end
-			if is_last then
-				return nil, "cancel", "conflict";
-			end
-		end
+		actor_affiliation = self:get_affiliation(actor);
 	end
-	self._affiliations[jid] = affiliation;
-	local role = self:get_default_role(affiliation);
-	local x = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user"})
-			:tag("item", {affiliation=affiliation or "none", role=role or "none"})
-				:tag("reason"):text(reason or ""):up()
-			:up();
-	local presence_type = nil;
-	if not role then -- getting kicked
-		presence_type = "unavailable";
-		if affiliation == "outcast" then
-			x:tag("status", {code="301"}):up(); -- banned
-		else
-			x:tag("status", {code="321"}):up(); -- affiliation change
+
+	-- First let's see if there are any problems with the affiliations given
+	-- in jid_affiliation
+	for jid, value in pairs(jid_affiliation) do
+		local affiliation = value["affiliation"];
+
+		jid = jid_bare(jid);
+		if affiliation == "none" then affiliation = nil; end
+		if affiliation and affiliation ~= "outcast" and affiliation ~= "owner" and affiliation ~= "admin" and affiliation ~= "member" then
+			return false, "modify", "not-acceptable";
 		end
-	end
-	local modified_nicks = {};
-	for nick, occupant in pairs(self._occupants) do
-		if jid_bare(occupant.jid) == jid then
-			if not role then -- getting kicked
-				self._occupants[nick] = nil;
-			else
-				occupant.affiliation, occupant.role = affiliation, role;
-			end
-			for jid,pres in pairs(occupant.sessions) do -- remove for all sessions of the nick
-				if not role then self._jid_nick[jid] = nil; end
-				local p = st.clone(pres);
-				p.attr.from = nick;
-				p.attr.type = presence_type;
-				p.attr.to = jid;
-				p:add_child(x);
-				self:_route_stanza(p);
-				if occupant.jid == jid then
-					modified_nicks[nick] = p;
+
+		local target_affiliation = self:get_affiliation(jid);
+		if target_affiliation == affiliation then
+			-- no change, no error checking necessary
+		else
+			if actor ~= true and actor_affiliation ~= "owner" then
+				if affiliation == "owner" or affiliation == "admin" or actor_affiliation ~= "admin" or target_affiliation == "owner" or target_affiliation == "admin" then
+					return false, "cancel", "not-allowed";
+				end
+			elseif target_affiliation == "owner" and jid_bare(actor) == jid then -- self change
+				local is_last = true;
+				for j, aff in pairs(self._affiliations) do if j ~= jid and aff == "owner" then is_last = false; break; end end
+				if is_last then
+					return false, "cancel", "conflict";
 				end
 			end
 		end
 	end
+
+	return true;
+end
+--- Updates the room affiliations by applying the ones given here.
+-- Takes the affiliations given in jid_affiliation and applies them to
+-- the room, overwriting a potentially existing affiliation for any given
+-- jid.
+-- @param jid_affiliation A table associating a jid with a table consisting
+--                        of two subkeys: `affilation` and `reason`. The jids
+--                        within must not be malformed.
+function room_mt:set_affiliations(actor, jid_affiliation, callback)
+	local can_set, err_type, err_condition = self:can_set_affiliations(actor, jid_affiliation)
+
+	if not can_set then
+		return false, err_type, err_condition;
+	end
+
+	local modified_nicks = {};
+	-- Now we can be sure that jid_affiliation causes no problems
+	-- We can actually set them
+	for jid, value in pairs(jid_affiliation) do
+		local affiliation = value["affiliation"];
+		local reason = value["reason"];
+
+		self._affiliations[jid] = affiliation;
+		local role = self:get_default_role(affiliation);
+		local x = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user"})
+				:tag("item", {affiliation=affiliation or "none", role=role or "none"})
+					:tag("reason"):text(reason or ""):up()
+				:up();
+		local presence_type = nil;
+		if not role then -- getting kicked
+			presence_type = "unavailable";
+			if affiliation == "outcast" then
+				x:tag("status", {code="301"}):up(); -- banned
+			else
+				x:tag("status", {code="321"}):up(); -- affiliation change
+			end
+		end
+		for nick, occupant in pairs(self._occupants) do
+			if jid_bare(occupant.jid) == jid then
+				if not role then -- getting kicked
+					self._occupants[nick] = nil;
+				else
+					occupant.affiliation, occupant.role = affiliation, role;
+				end
+				for jid,pres in pairs(occupant.sessions) do -- remove for all sessions of the nick
+					if not role then self._jid_nick[jid] = nil; end
+					local p = st.clone(pres);
+					p.attr.from = nick;
+					p.attr.type = presence_type;
+					p.attr.to = jid;
+					p:add_child(x);
+					self:_route_stanza(p);
+					if occupant.jid == jid then
+						modified_nicks[nick] = p;
+					end
+				end
+			end
+		end
+	end
+
 	if self.save then self:save(); end
 	if callback then callback(); end
 	for nick,p in pairs(modified_nicks) do
@@ -1039,6 +1105,9 @@
 	end
 	return true;
 end
+function room_mt:set_affiliation(actor, jid, affiliation, callback, reason)
+	return self.set_affiliations(actor, { [jid] = { ["affiliation"] = affiliation, ["reason"] = reason } }, callback)
+end
 
 function room_mt:get_role(nick)
 	local session = self._occupants[nick];
@@ -1062,42 +1131,68 @@
 	end
 	return nil, "cancel", "not-allowed";
 end
-function room_mt:set_role(actor, occupant_jid, role, callback, reason)
-	if role == "none" then role = nil; end
-	if role and role ~= "moderator" and role ~= "participant" and role ~= "visitor" then return nil, "modify", "not-acceptable"; end
-	local allowed, err_type, err_condition = self:can_set_role(actor, occupant_jid, role);
-	if not allowed then return allowed, err_type, err_condition; end
-	local occupant = self._occupants[occupant_jid];
-	local x = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user"})
-			:tag("item", {affiliation=occupant.affiliation or "none", nick=select(3, jid_split(occupant_jid)), role=role or "none"})
-				:tag("reason"):text(reason or ""):up()
-			:up();
-	local presence_type = nil;
-	if not role then -- kick
-		presence_type = "unavailable";
-		self._occupants[occupant_jid] = nil;
-		for jid in pairs(occupant.sessions) do -- remove for all sessions of the nick
-			self._jid_nick[jid] = nil;
-		end
-		x:tag("status", {code = "307"}):up();
-	else
-		occupant.role = role;
-	end
-	local bp;
-	for jid,pres in pairs(occupant.sessions) do -- send to all sessions of the nick
-		local p = st.clone(pres);
-		p.attr.from = occupant_jid;
-		p.attr.type = presence_type;
-		p.attr.to = jid;
-		p:add_child(x);
-		self:_route_stanza(p);
-		if occupant.jid == jid then
-			bp = p;
+
+function room_mt:can_set_roles(actor, jidnick_role)
+	for jidnick, role in pairs(jidnick_role) do
+		if role == "none" then role = nil; end
+		if role and role ~= "moderator" and role ~= "participant" and role ~= "visitor" then return false, "modify", "not-acceptable"; end
+		local can_set, err_type, err_condition = self:can_set_role(actor, jidnick, role)
+		if not can_set then
+			return false, err_type, err_condition;
 		end
 	end
+
+	return true;
+end
+--- Updates the room roles by applying the ones given here.
+-- Takes the roles given in jidnick_role and applies them to
+-- the room, overwriting a potentially existing role for any given
+-- jid.
+-- @param jidnick_role A table associating a jid/nick with a table consisting
+--                     of two subkeys: `role` and `reason`. The jids within
+--                     must not be malformed.
+function room_mt:set_roles(actor, jidnick_role, callback)
+	local allowed, err_type, err_condition = self:can_set_roles(actor, jidnick_role);
+	if not allowed then return allowed, err_type, err_condition; end
+
+	local modified_nicks = {};
+	for jidnick, value in pairs(jidnick_role) do
+		local occupant_jid = jidnick;
+		local role = value["role"];
+		local reason = value["reason"];
+
+		local occupant = self._occupants[occupant_jid];
+		local x = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user"})
+				:tag("item", {affiliation=occupant.affiliation or "none", nick=select(3, jid_split(occupant_jid)), role=role or "none"})
+					:tag("reason"):text(reason or ""):up()
+				:up();
+		local presence_type = nil;
+		if not role then -- kick
+			presence_type = "unavailable";
+			self._occupants[occupant_jid] = nil;
+			for jid in pairs(occupant.sessions) do -- remove for all sessions of the nick
+				self._jid_nick[jid] = nil;
+			end
+			x:tag("status", {code = "307"}):up();
+		else
+			occupant.role = role;
+		end
+		for jid,pres in pairs(occupant.sessions) do -- send to all sessions of the nick
+			local p = st.clone(pres);
+			p.attr.from = occupant_jid;
+			p.attr.type = presence_type;
+			p.attr.to = jid;
+			p:add_child(x);
+			self:_route_stanza(p);
+			if occupant.jid == jid then
+				modified_nicks[occupant_jid] = p;
+			end
+		end
+	end
+
 	if callback then callback(); end
-	if bp then
-		self:broadcast_except_nick(bp, occupant_jid);
+	for nick,p in pairs(modified_nicks) do
+		self:broadcast_except_nick(p, nick);
 	end
 	return true;
 end

Reply via email to