Standalone signaling support #366

fancycode · 2017-07-20T11:47:17Z

This PR adds support for using a standalone signaling server.

See https://github.com/nextcloud/spreed/wiki/Spreed-Signaling-API for the documentation of message formats. In a follow-up PR, the same messages will be implemented for the PHP backend, so there is only one API that clients need to implement.

@nickvergessen @Ivansss

codecov · 2017-07-20T13:57:10Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d7ef5ef). Click here to learn what that means.
The diff coverage is 1.32%.

@@            Coverage Diff            @@
##             master     #366   +/-   ##
=========================================
  Coverage          ?   10.32%           
  Complexity        ?      394           
=========================================
  Files             ?       23           
  Lines             ?     1975           
  Branches          ?        0           
=========================================
  Hits              ?      204           
  Misses            ?     1771           
  Partials          ?        0

Impacted Files	Coverage Δ	Complexity Δ
appinfo/app.php	`0% <ø> (ø)`	`0 <0> (?)`
lib/AppInfo/Application.php	`0% <0%> (ø)`	`3 <2> (?)`
lib/Controller/PageController.php	`0% <0%> (ø)`	`20 <4> (?)`
lib/Controller/RoomController.php	`0% <0%> (ø)`	`94 <0> (?)`
lib/Controller/AppSettingsController.php	`0% <0%> (ø)`	`19 <18> (?)`
appinfo/routes.php	`0% <0%> (ø)`	`0 <0> (?)`
lib/BackendNotifier.php	`0% <0%> (ø)`	`13 <13> (?)`
lib/Controller/SignallingController.php	`0% <0%> (ø)`	`37 <16> (?)`
lib/Manager.php	`0% <0%> (ø)`	`45 <1> (?)`
lib/Room.php	`0% <0%> (ø)`	`42 <3> (?)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7ef5ef...85bdea6. Read the comment docs.

fancycode · 2017-07-25T10:38:33Z

@nickvergessen @Ivansss this is now ready for some testing 😃

fancycode · 2017-08-31T10:19:04Z

@nickvergessen @Ivansss this is now pending for quite some time - would be great to get some feedback...

nickvergessen · 2017-09-01T14:27:48Z

I will have a look on monday/tuesday, sorry conf was too busy

nickvergessen · 2017-09-05T12:01:31Z

lib/Controller/AppSettingsController.php

+				if (!filter_var($server, FILTER_VALIDATE_URL)) {
+					return array('data' =>
+						array('message' =>
+							(string) $this->l10n->t('Invalid signaling server url.')


cast is not required anymore

For consistency I would rather keep this aligned with surrounding code and remove the cast here and in other locations in a follow-up PR.

Yeah, same as in the comment below, just ignore for now ;)

nickvergessen · 2017-09-05T12:02:05Z

lib/Controller/AppSettingsController.php

+			$servers = explode('\n', $signaling_server);
+			foreach($servers as $server) {
+				if (!filter_var($server, FILTER_VALIDATE_URL)) {
+					return array('data' =>


you can use the short array syntax [ ]

Ah, just saw you only copy pasted, so okay

nickvergessen · 2017-09-05T12:05:00Z

lib/Controller/AppSettingsController.php

@@ -55,8 +55,11 @@ public function __construct($appName,
 	 * @param string $stun_server
 	 * @param string $turn_server
 	 * @param string $turn_server_secret
+	 * @param string $signaling_server
+	 * @param string $signaling_secret
+	 * @param string $signaling_skip_verify_cert


order is mixed up
can you also add $turn_server_protocols to the docs?

nickvergessen · 2017-09-05T12:06:54Z

lib/Controller/AppSettingsController.php

+		}
+
+		$currentSignalingSkipVerifyCert = $this->config->getAppValue('spreed', 'signaling_skip_verify_cert', '');
+		if ($signaling_skip_verify_cert === null) {


$signaling_skip_verify_cert = (string) $signaling_skip_verify_cert;

Sorry, I don't understand what you want to have changed here.

You set the variable to an empty string when it is null, so you can also just cast it to string, getting rid of the if-block. Should have the same result?

php magic 🙈

nickvergessen · 2017-09-05T12:12:20Z

lib/Controller/RoomController.php

@@ -305,6 +310,11 @@ protected function createOneToOneRoom($targetUserName) {
 			$room->addParticipant($targetUser->getUID(), Participant::OWNER);
 			$this->createNotification($currentUser, $targetUser, $room);

+			$this->backend->roomInvited($room, [


I think I would move this into the methods of the Room class.
For better decoupling we could also trigger a hook? This would allow any app to listen to those events and provide a signaling backend?

This is now updated to use events in the latest commit.

nickvergessen · 2017-09-05T12:14:01Z

lib/Controller/SignallingController.php

@@ -76,6 +82,11 @@ public function __construct($appName,
 	 * @return JSONResponse
 	 */
 	public function signalling($messages) {
+		$signaling = $this->config->getSignalingServer();
+		if (!empty($signaling)) {
+			throw new \Exception('Internal signaling disabled.');


Should return new JSONResponse('Internal signaling disabled.', Http::STATUS_BAD_REQUEST); instead of throwing?

nickvergessen · 2017-09-05T12:15:57Z

lib/Controller/SignallingController.php

+	 * @return JSONResponse
+	 */
+	public function backend() {
+		$json = file_get_contents('php://input');


can't you use $this->request ?

$this->request is a OCP\IRequest which only provides access to the request variables / -headers but I need the body of the request. Maybe there is a better way to get that - if so I didn't find it ;-)

Or you post a json body to the server using parameters instead of the body? ;)

You mean sending a body body=the-json-object and then using the value of variable body? That sounds hackish ;-)

nickvergessen · 2017-09-05T12:20:32Z

lib/Controller/BackendController.php

+use OCP\IRequest;
+use OCP\Security\ISecureRandom;
+
+class BackendController extends Controller {


This controller has no routes, drop it?
Or you forgot to commit something?

No longer a controller now.

nickvergessen · 2017-09-05T12:21:06Z

lib/Controller/BackendController.php

+	 * @param Room $room
+	 * @param array $userIds
+	 */
+	public function roomInvited($room, $userIds) {


this is only reachable for admins, right?

nickvergessen · 2017-09-05T12:35:29Z

The main question is at which point the things should be decoupled.
I guess currently this app contains the bridge code as well, right?
So that the "external" signalings can focus on signaling instead of also having to do the php app code to listen to the events, etc?

nickvergessen · 2017-09-06T13:57:12Z

lib/Room.php

+				'type' => $participantType,
+				'sessionId' => $sessionId,
+			]));
+		foreach ($participants as &$participant) {


remove the reference &

nickvergessen · 2017-09-06T13:58:35Z

lib/Room.php

+				'sessionId' => $sessionId,
+			]));
+		foreach ($participants as &$participant) {
+			$query = $this->db->getQueryBuilder();


Please create this outside of the foreach, use $query->createParameter('userId') and so on, and inside the loop just call setParameter('userId', $participant)->setParameter(....); $query->execute();

nickvergessen · 2017-09-06T13:59:57Z

lib/AppInfo/Application.php

+		$notifier = $this->getContainer()->query(BackendNotifier::class);
+
+		$dispatcher = $this->getContainer()->getServer()->getEventDispatcher();
+		$dispatcher->addListener('\OCA\Spreed\Room::postAddParticipants', function($event) use ($notifier) {


I'd use ->addListener(Room::class . '::postAddParticipants') like we do in other places as well already

Changed. I based my code on the dav app, where the complete strings are used - using the classes makes more sense though!

nickvergessen · 2017-09-06T14:00:36Z

lib/Room.php

@@ -146,6 +152,8 @@ public function getParticipant($userId) {
 	}

 	public function deleteRoom() {
+		$this->dispatcher->dispatch('\OCA\Spreed\Room::preDeleteRoom',


self::class . '::preDeleteRoom'

fancycode · 2017-09-06T15:02:05Z

Rebased to current master.