Added parameter to disallow creating of rooms with public access #8082

pboguslawski · 2022-10-02T23:11:34Z

Added spreed application config parameter public_rooms_allowed to disallow
creating of rooms with public (guest via link) access (i.e. for internal
Nextcloud setups). Use

occ config:app:set spreed public_rooms_allowed --value 'yes'

to allow (default if not set) and

occ config:app:set spreed public_rooms_allowed --value 'no'

to disallow creating of rooms with public access.

Related: #521
Author-Change-Id: IB#1126215
Signed-off-by: Pawel Boguslawski pawel.boguslawski@ib.pl

Added spreed application config parameter `public_rooms_allowed` to disallow creating of rooms with public (guest via link) access (i.e. for internal Nextcloud setups). Use ``` occ config:app:set spreed public_rooms_allowed --value 'yes' ``` to allow (default if not set) and ``` occ config:app:set spreed public_rooms_allowed --value 'no' ``` to disallow creating of rooms with public access. Related: nextcloud#521 Author-Change-Id: IB#1126215 Signed-off-by: Pawel Boguslawski <pawel.boguslawski@ib.pl>

nickvergessen · 2022-10-03T08:02:47Z

lib/Manager.php

+
+		// Force group room if public room was requested and public rooms are disallowed.
+		if (($type === Room::TYPE_PUBLIC) && ($this->config->getAppValue('spreed', 'public_rooms_allowed', 'yes') === 'no')) {
+			$type = Room::TYPE_GROUP;


Not sure if we should simply convert it. Or just error with a 400 Bad Request and a message the clients can display.

Conversion was choosen because spreed code ignores errors returned by modified functions and this seems to be safer. Conversion should never happen because function should be not available in UI also (and if bad guys will try to modify POST, will get group conversation which should not hurt regular users).

That assumes all clients got updated in the meantime :)

Sorry, we use web UI only - feel free to extend it for other required areas if necessary (not experienced enough in nc devel to propose it now).

You can fix it with one step by moving it to RoomService::createConversation and throwing an exception or basically adjust this list here:

spreed/lib/Service/RoomService.php

Lines 141 to 147 in 2200896

if (!\in_array($type, [

Room::TYPE_GROUP,

Room::TYPE_PUBLIC,

Room::TYPE_CHANGELOG,

], true)) {

throw new InvalidArgumentException('type');

}

type is property of Room not RoomService so verification should take place in Room class to avoid future problems (i.e. code outside of RoomService will create/change room of disabled type). Seems that now Room::setType and Room constructor are the best places for this verification but code using it should properly handle errors when disabled type is passed. Please let me know if its possible to just throw exception in Room methods mentioned above without messing with room type conversions (there should not be any other trash left after such exception - not sure if it would be safe now).

nickvergessen · 2022-10-03T08:04:02Z

lib/Room.php

+		// Force group room if public room was requested and public rooms are disallowed.
+		if (($type === self::TYPE_PUBLIC) && ($this->config->getAppValue('spreed', 'public_rooms_allowed', 'yes') === 'no')) {
+			$type = self::TYPE_GROUP;
+		}


This is not correct. You want to do this change on the RoomService, not the model.
But see comment above.

Modified places where choosen to avoid problems with lack of return errors handling. If you still recommend moving it elswhere - please suggest detailed change.

As per above, you should do it only in the RoomService

Don't see any mention of setType in RoomService and this method may be called so must verify room type also.

nickvergessen · 2022-10-03T08:04:46Z

lib/Settings/Admin/AdminSettings.php

+
+		$this->initialState->provideInitialState(
+			'public_rooms_allowed',
+			$this->serverConfig->getAppValue('spreed', 'public_rooms_allowed', 'yes') === 'yes'


We need to expose this to the mobile clients as a capability so they can also hide the features accordingly.

This mod was created for web UI. Not experienced in nextcloud devel nor using mobile app so please suggest required changes if necessary.

Basically add a line like this which in addition also checks your config:

spreed/lib/Capabilities.php

Line 151 in 91d3bd8

$capabilities['config']['conversations']['can-create'] = $user instanceof IUser && !$this->talkConfig->isNotAllowedToCreateConversations($user);

Not experienced with capabilities in Nextcloud - please create change suggestion and will just accept it to this PR or make separate PR to adjust it.

marcoambrosini

tested and works 👌

nickvergessen

Also RoomService::setType() should check the config.
RoomController::addParticipantToRoom should not allow to add email addresses to a conversation (which also makes the room public)
Since emails are then not allowed anymore, we should also not autocomplete them here:

spreed/src/services/conversationsService.js

Line 83 in d01450f

shareTypes.push(SHARE.TYPE.EMAIL)

nickvergessen · 2022-10-03T11:39:18Z

lib/Manager.php

+
+		// Force group room if public room was requested and public rooms are disallowed.
+		if (($type === Room::TYPE_PUBLIC) && ($this->config->getAppValue('spreed', 'public_rooms_allowed', 'yes') === 'no')) {
+			$type = Room::TYPE_GROUP;


You can fix it with one step by moving it to RoomService::createConversation and throwing an exception or basically adjust this list here:

spreed/lib/Service/RoomService.php

Lines 141 to 147 in 2200896

if (!\in_array($type, [

Room::TYPE_GROUP,

Room::TYPE_PUBLIC,

Room::TYPE_CHANGELOG,

], true)) {

throw new InvalidArgumentException('type');

}

nickvergessen · 2022-10-03T11:39:42Z

lib/Room.php

+		// Force group room if public room was requested and public rooms are disallowed.
+		if (($type === self::TYPE_PUBLIC) && ($this->config->getAppValue('spreed', 'public_rooms_allowed', 'yes') === 'no')) {
+			$type = self::TYPE_GROUP;
+		}


As per above, you should do it only in the RoomService

nickvergessen · 2022-10-03T11:41:14Z

lib/Settings/Admin/AdminSettings.php

+
+		$this->initialState->provideInitialState(
+			'public_rooms_allowed',
+			$this->serverConfig->getAppValue('spreed', 'public_rooms_allowed', 'yes') === 'yes'


Basically add a line like this which in addition also checks your config:

spreed/lib/Capabilities.php

Line 151 in 91d3bd8

$capabilities['config']['conversations']['can-create'] = $user instanceof IUser && !$this->talkConfig->isNotAllowedToCreateConversations($user);

pboguslawski · 2022-10-14T13:12:55Z

Also RoomService::setType() should check the config.

RoomController::addParticipantToRoom should not allow to add email addresses to a conversation (which also makes the room public)

Since emails are then not allowed anymore, we should also not autocomplete them here:

spreed/src/services/conversationsService.js

Line 83 in d01450f

shareTypes.push(SHARE.TYPE.EMAIL)

Verification should be placed in Room class in all methods capable of setting/changing type (now constructor and setType probably) but code using Room must be prepared for errors/exceptions (not sure if already is) so please propose better solution than conversion proposed in this change (as suggestions or separate PR).

…owed Fixes: a38fdfd Related: nextcloud#8082 (review) Author-Change-Id: IB#1126215 Signed-off-by: Pawel Boguslawski <pawel.boguslawski@ib.pl>

pboguslawski · 2022-10-17T15:20:56Z

4c645ce disables adding e-mails to room when public rooms are disabled.

nickvergessen · 2022-10-17T15:24:20Z

Sorry for the lack of responsiveness lately.
I still have your PR on my todo list and hopefully will reach it this week.

…owed Fixes: a38fdfd Related: #8082 (review) Author-Change-Id: IB#1126215 Signed-off-by: Pawel Boguslawski <pawel.boguslawski@ib.pl>

nickvergessen · 2022-11-16T12:11:48Z

Hi there, I copied your branch into our organisation and sent you an invite.
This allows us to better collaborate on the feature.

I rebased it on latest master and did some fixes and changes.
New pull request is here: #8357

pboguslawski added 2 commits October 3, 2022 01:03

Merge master into master-IB#1126215

09e53ea

nickvergessen added this to the 💟 Next Major (26) milestone Oct 3, 2022

nickvergessen added 3. to review enhancement feature: chat 💬 Chat and system messages feature: settings ⚙️ Settings and config related issues feature: api 🛠️ OCS API for conversations, chats and participants feature: frontend 🖌️ "Web UI" client labels Oct 3, 2022

nickvergessen requested review from marcoambrosini and nickvergessen October 3, 2022 08:02

nickvergessen requested changes Oct 3, 2022

View reviewed changes

marcoambrosini approved these changes Oct 3, 2022

View reviewed changes

nickvergessen requested changes Oct 3, 2022

View reviewed changes

Adding e-mail addresses to room disabled when public rooms are disall…

4c645ce

…owed Fixes: a38fdfd Related: nextcloud#8082 (review) Author-Change-Id: IB#1126215 Signed-off-by: Pawel Boguslawski <pawel.boguslawski@ib.pl>

nickvergessen mentioned this pull request Nov 16, 2022

Allow to disable public conversations #8357

Draft

18 tasks

nickvergessen closed this Nov 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added parameter to disallow creating of rooms with public access #8082

Added parameter to disallow creating of rooms with public access #8082

pboguslawski commented Oct 2, 2022

nickvergessen Oct 3, 2022

pboguslawski Oct 3, 2022

nickvergessen Oct 3, 2022

pboguslawski Oct 3, 2022 •

edited

Loading

nickvergessen Oct 3, 2022

pboguslawski Oct 14, 2022

nickvergessen Oct 3, 2022

pboguslawski Oct 3, 2022

nickvergessen Oct 3, 2022

pboguslawski Oct 14, 2022

nickvergessen Oct 3, 2022

pboguslawski Oct 3, 2022 •

edited

Loading

nickvergessen Oct 3, 2022

pboguslawski Oct 14, 2022

marcoambrosini left a comment

nickvergessen left a comment

nickvergessen Oct 3, 2022

nickvergessen Oct 3, 2022

nickvergessen Oct 3, 2022

pboguslawski commented Oct 14, 2022

pboguslawski commented Oct 17, 2022 •

edited

Loading

nickvergessen commented Oct 17, 2022

nickvergessen commented Nov 16, 2022

	if (!\in_array($type, [
	Room::TYPE_GROUP,
	Room::TYPE_PUBLIC,
	Room::TYPE_CHANGELOG,
	], true)) {
	throw new InvalidArgumentException('type');
	}

Added parameter to disallow creating of rooms with public access #8082

Added parameter to disallow creating of rooms with public access #8082

Conversation

pboguslawski commented Oct 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pboguslawski Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pboguslawski Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoambrosini left a comment

Choose a reason for hiding this comment

nickvergessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pboguslawski commented Oct 14, 2022

pboguslawski commented Oct 17, 2022 • edited Loading

nickvergessen commented Oct 17, 2022

nickvergessen commented Nov 16, 2022

pboguslawski Oct 3, 2022 •

edited

Loading

pboguslawski Oct 3, 2022 •

edited

Loading

pboguslawski commented Oct 17, 2022 •

edited

Loading