Add TURN server to personal settings. by fancycode · Pull Request #84 · nextcloud/spreed

fancycode · 2016-11-14T10:51:43Z

This implements the second part of #2 to add TURN support. For now each user
has to have an account on the TURN server. Support for shared credentials will
be added in a separate PR.

@nickvergessen @LukasReschke

fancycode · 2016-11-14T11:02:28Z

personal.php

+$uid = \OC::$server->getUserSession()->getUser()->getUID();
+
+$settings = Util::getTurnSettings($config, $uid);
+//server":"1","username":"2","password":"3","protocols


Whoops, some debugging leftover - will remove together with other feedback you might have.

LukasReschke

Looks good. Some remarks below.

I'm not entirely sure if it is really required that users can configure their own TURN server as well. But well 🙈

LukasReschke · 2016-11-14T13:42:47Z

lib/Controller/PersonalSettingsController.php

+		);
+	}
+
+}


All hail to unit tests 😉 – At least as it is a new file. :-)

Can you recommend an existing unittest for a controller I should use as starting point?

Absolutely. Gimme some mins ;-)

https://github.com/nextcloud/server/blob/03859799e617a72df3a16a8ecf18d1bf049db3e1/apps/bruteforcesettings/tests/Controller/IPWhitelistControllerTest.php

And https://github.com/nextcloud/server/blob/03859799e617a72df3a16a8ecf18d1bf049db3e1/apps/bruteforcesettings/lib/Controller/IPWhitelistController.php :-)

Added a test in b6b263a

LukasReschke · 2016-11-14T13:44:05Z

personal.php

+
+use OCA\Spreed\Util;
+
+\OC_Util::checkLoggedIn();


Not required anymore since ownCloud 8 or so :-)

LukasReschke · 2016-11-14T13:44:40Z

personal.php

+$settings = Util::getTurnSettings($config, $uid);
+//server":"1","username":"2","password":"3","protocols
+$tmpl->assign('turnSettings', $settings);
+return $tmpl->fetchPage();


Would prefer something like https://github.com/nextcloud/activity/blob/master/personal.php instead :-)

Thanks, will update.

LukasReschke · 2016-11-14T13:47:54Z

lib/Util.php

        return $config->getAppValue('spreed', 'stun_server', 'stun.l.google.com:19302');
    }

+    public static function getTurnSettings(IConfig $config, string $uid) {


Seems kinda strange to always have to inject IConfig and so on. Maybe make it non-static? :-)

I will create a separate PR to change this, also for the existing code.

LukasReschke · 2016-11-15T08:40:43Z

lib/Controller/PersonalSettingsController.php

+		parent::__construct($appName, $request);
+		$this->l10n = $l10n;
+		$this->config = $config;
+		$this->user = $userSession && $userSession->isLoggedIn() ? $userSession->getUser() : false;


If you just want the UID just use $UserId and it will automatically inject the current user id. It's like the appname thingy :)

Well well, so much to learn... ;-)

nickvergessen · 2016-11-15T08:50:51Z

lib/AppInfo/Application.php

+        parent::__construct('spreed');
+        $container = $this->getContainer();
+
+        $container->registerAlias('PersonalSettingsController', PersonalSettingsController::class);


this should not be needed

Hmm, if I remove it, loading the settings fails with this error (from nextcloud.log):

"Exception: {\"Exception\":\"OCP\\\\AppFramework\\\\QueryException\",\"Message\":\"Could not resolve PersonalSettingsController! Class PersonalSettingsController does not exist\",\"Code\":0,\"Trace\":\"#0 \\\/srv\\\/www\\\/lib\\\/private\\\/AppFramework\\\/Utility\\\/SimpleContainer.php(117): OC\\\\AppFramework\\\\Utility\\\\SimpleContainer->resolve('PersonalSetting...')\\n#1 \\\/srv\\\/www\\\/lib\\\/private\\\/AppFramework\\\/DependencyInjection\\\/DIContainer.php(540): OC\\\\AppFramework\\\\Utility\\\\SimpleContainer->query('PersonalSetting...')\\n#2 \\\/srv\\\/www\\\/apps\\\/spreed\\\/personal.php(24): OC\\\\AppFramework\\\\DependencyInjection\\\\DIContainer->query('PersonalSetting...')\\n#3 \\\/srv\\\/www\\\/lib\\\/private\\\/legacy\\\/app.php(757): include('\\\/srv\\\/www\\\/apps\\\/s...')\\n#4 \\\/srv\\\/www\\\/settings\\\/personal.php(184): OC_App::getForms('personal')\\n#5 \\\/srv\\\/www\\\/lib\\\/private\\\/Route\\\/Route.php(155) : runtime-created function(1): require_once('\\\/srv\\\/www\\\/settin...')\\n#6 [internal function]: __lambda_func()\\n#7 \\\/srv\\\/www\\\/lib\\\/private\\\/Route\\\/Router.php(298): call_user_func('\\\\x00lambda_603', Array)\\n#8 \\\/srv\\\/www\\\/lib\\\/base.php(1000): OC\\\\Route\\\\Router->match('\\\/settings\\\/perso...')\\n#9 \\\/srv\\\/www\\\/index.php(40): OC::handleRequest()\\n#10 {main}\",\"File\":\"\\\/srv\\\/www\\\/lib\\\/private\\\/AppFramework\\\/Utility\\\/SimpleContainer.php\",\"Line\":102}"

MorrisJobke · 2016-11-15T16:45:15Z

Just FYI: I rebased this ;)

MorrisJobke · 2016-11-15T16:55:27Z

appinfo/app.php

 \OCP\Util::connectHook('OC_User', 'post_deleteUser', \OCA\Spreed\Util::class, 'deleteUser');

-OC_App::registerPersonal('spreed', 'personal');
+\OCP\App::registerPersonal('spreed', 'personal');


@fancycode This fixes the app:check-code CI check ;)

MorrisJobke · 2016-11-15T16:55:51Z

I tested this and it works 👍

Code looks good :)

jancborchardt · 2016-11-15T18:26:52Z

I also fixed the naming to be »Spreed video calls« everywhere (appname and settings headers for both personal and admin settings) because we were calling it different names. :)

fancycode · 2016-11-16T08:53:58Z

Squashed a couple of commits and added Signed-off-by markers.

@LukasReschke please review

codecov-io · 2016-11-16T10:39:42Z

Current coverage is 10.92% (diff: 46.15%)

Merging #84 into master will increase coverage by 3.98%

@@            master        #84   diff @@
=========================================
  Files           11         13     +2   
  Lines          577        641    +64   
  Methods         39         44     +5   
  Messages         0          0          
  Branches         0          0          
=========================================
+ Hits            40         70    +30   
- Misses         537        571    +34   
  Partials         0          0

Powered by Codecov. Last update 7a743fe...d02b4a4

jancborchardt · 2016-11-16T19:40:40Z

So I know you @MorrisJobke @fancycode talked about why this is in the personal settings. But it is a really strange UX to have it there. That would mean every single user would have to specify it, and many even technical people probably never heard about it.

So, let’s move it to the Admin settings please? ;)

fancycode · 2016-11-16T19:58:27Z

Well, the settings as-is (i.e. username and password) is per user, so the personal settings is the right place.

For the Admin area we should have an additional setting to configure a shared secret to be used, but that requires some application-level code to generate TURN credentials dynamically for the different participants. I'm planning to add this in a separate PR.

IMHO both settings make sense depending on the use case.

jancborchardt · 2016-11-18T14:14:54Z

@LukasReschke your call please :)

(UX-wise I’d prefer if we start with it being in Admin settings first, and only then add per-user settings. Cause then they can edit based on the configured TURN server.)

LukasReschke · 2016-11-18T14:17:27Z

Agreeing with @jancborchardt. User settings do look rather confusing. An instance-wide setting seems way more user-friendly.

For the Admin area we should have an additional setting to configure a shared secret to be used, but that requires some application-level code to generate TURN credentials dynamically for the different participants. I'm planning to add this in a separate PR.

Based on https://github.com/andyet/signalmaster/blob/e98d388a2107330948616661064cb329d2749314/sockets.js#L105-L124 which is the signalling server that we ported this should likely be done at

spreed/lib/Controller/SignallingController.php

Lines 97 to 105 in 4911cc8

    
           case 'stunservers': 
        
           	$response = []; 
        
           	$stunServer = Util::getStunServer($this->config); 
        
           	if ($stunServer) { 
        
           		array_push($response, [ 
        
           			'url' => 'stun:' . $stunServer, 
        
           		]); 
        
           	} 
        
           	break;

.

MorrisJobke · 2016-11-18T14:24:30Z

Cause then they can edit based on the configured TURN server.)

That's not how these are usually set up. Either the admin has one which creates the profiles for the users on demand or the users have a completely different one (also every user could have a different one very likely if they have one at all)

MorrisJobke · 2016-11-21T13:37:31Z

I will rebase this one here to be able to merge it. Then I will look into the admin setting for the TURN server.

This implements the second part of #2 to add TURN support. For now each user has to have an account on the TURN server. Support for shared credentials will be added in a separate PR. Signed-off-by: Joachim Bauch <[email protected]>

Signed-off-by: Joachim Bauch <[email protected]>

Signed-off-by: Morris Jobke <[email protected]>

Signed-off-by: Jan-Christoph Borchardt <[email protected]>

fancycode · 2016-11-21T13:51:59Z

For the admin setting, I would recommend to use the shared-secret mode and generate username/password similar to what we are doing in spreed-webrtc:
https://github.com/strukturag/spreed-webrtc/blob/e18dd5c93382f4e15fcd97e921a680cd2494622f/go/channelling/hub.go#L94

MorrisJobke · 2016-11-21T14:06:17Z

For the admin setting, I would recommend to use the shared-secret mode and generate username/password similar to what we are doing in spreed-webrtc:

Yes. That was the plan :)

fancycode · 2016-11-21T14:11:51Z

Great 👍

MorrisJobke · 2016-11-21T16:04:48Z

Failing CI is also on master (eslint) I will fix this later.

MorrisJobke · 2016-11-21T16:04:59Z

Talked with @LukasReschke and he is fine -> merge

0.3.0

fancycode commented Nov 14, 2016

View reviewed changes

LukasReschke requested changes Nov 14, 2016

View reviewed changes

LukasReschke reviewed Nov 15, 2016

View reviewed changes

nickvergessen reviewed Nov 15, 2016

View reviewed changes

fancycode force-pushed the personal_turn_settings branch from 56094da to 2a3229d Compare November 15, 2016 09:02

MorrisJobke force-pushed the personal_turn_settings branch from fe97298 to 45e0267 Compare November 15, 2016 16:35

MorrisJobke approved these changes Nov 15, 2016

View reviewed changes

MorrisJobke added this to the 1.0.0 milestone Nov 15, 2016

MorrisJobke added the enhancement label Nov 15, 2016

fancycode force-pushed the personal_turn_settings branch from 214b03e to d02b4a4 Compare November 16, 2016 07:59

MorrisJobke mentioned this pull request Nov 16, 2016

Nextcloud 11 release review nextcloud/server#2134

Closed

67 tasks

fancycode and others added 6 commits November 21, 2016 14:41

Add TURN server to personal settings.

ecb1823

This implements the second part of #2 to add TURN support. For now each user has to have an account on the TURN server. Support for shared credentials will be added in a separate PR. Signed-off-by: Joachim Bauch <[email protected]>

Move personal settings template generation to controller.

d8843be

Signed-off-by: Joachim Bauch <[email protected]>

Get "$userId" through injection.

af081dc

Signed-off-by: Joachim Bauch <[email protected]>

Added first unittest for "PersonalSettingsController".

ef1451d

Signed-off-by: Joachim Bauch <[email protected]>

Use public API instead of private methods

3ada9c7

Signed-off-by: Morris Jobke <[email protected]>

normalize name to Spreed video calls

df3f436

Signed-off-by: Jan-Christoph Borchardt <[email protected]>

MorrisJobke force-pushed the personal_turn_settings branch from d02b4a4 to df3f436 Compare November 21, 2016 13:41

MorrisJobke merged commit e7f2cb4 into master Nov 21, 2016

MorrisJobke deleted the personal_turn_settings branch November 21, 2016 16:05

MorrisJobke mentioned this pull request Nov 21, 2016

Add admin setting to specify a TURN server and a shared secret #120

Merged

jancborchardt mentioned this pull request Nov 25, 2016

Add input fields for configuration page #2

Closed

2 tasks

marcoambrosini pushed a commit that referenced this pull request Oct 9, 2019

Merge pull request #84 from nextcloud/3.0

b1c3308

0.3.0

fancycode mentioned this pull request Apr 21, 2020

Turnserver compatibility user/password instead static-auth #3301

Closed

BelgarathS mentioned this pull request Oct 1, 2025

Notification Sounds not being played for talk messages #16073

Closed

+              		);
+              	}
+              }

Comments

Conversation

fancycode commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukasReschke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukasReschke Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Nov 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Nov 15, 2016

Uh oh!

jancborchardt commented Nov 15, 2016

Uh oh!

fancycode commented Nov 16, 2016

Uh oh!

codecov-io commented Nov 16, 2016

Current coverage is 10.92% (diff: 46.15%)

Uh oh!

jancborchardt commented Nov 16, 2016

Uh oh!

fancycode commented Nov 16, 2016

Uh oh!

jancborchardt commented Nov 18, 2016

Uh oh!

LukasReschke commented Nov 18, 2016

Uh oh!

MorrisJobke commented Nov 18, 2016

Uh oh!

MorrisJobke commented Nov 21, 2016

Uh oh!

fancycode commented Nov 21, 2016

Uh oh!

MorrisJobke commented Nov 21, 2016

Uh oh!

fancycode commented Nov 21, 2016

Uh oh!

MorrisJobke commented Nov 21, 2016

Uh oh!

MorrisJobke commented Nov 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

LukasReschke Nov 15, 2016 •

edited

Loading