Room passwords #402

nickvergessen · 2017-09-11T15:20:07Z

Add password field to room table
Allow to set the password via API
Require the field when joining a public call
UI to set the password
UI for entering the password
Adjust docs
Only allow participant list after entering the password
Make "self joined users" leave the participant list when they leave the room
Integration tests

leonklingele-work · 2017-09-19T10:05:49Z

lib/Room.php

@@ -284,6 +321,10 @@ public function enterRoomAsUser($userId) {
 		$result = $query->execute();

 		if ($result === 0) {
+			if ($this->getPassword() !== '' && $this->getPassword() !== $password) {


Use hash_equals instead of !== to compare secret with user-provided value.

Well we don't compare hashes yet. Also see the note in docs:
http://php.net/manual/en/function.hash-equals.php

Both arguments must be of the same length to be compared successfully. When arguments of differing length are supplied, FALSE is returned immediately and the length of the known string may be leaked in case of a timing attack.

Also I'm pretty sure you can not do timing attacks against nextcloud apis 🙈

hash_equals can be used to compare any kind of strings, be it hashes or PINs.
Leaking the length of the secret is fine, it is specified in this open source code after all.

Also I'm pretty sure you can not do timing attacks against nextcloud apis

I wouldn't bet on that.

Leaking the length of the secret is fine, it is specified in this open source code after all.

There is no secret neither PINs, we have a room password here

The length is unknown, since it will be provided by the owner of the room

It's not specified in any code Oo?

Don't get me wrong, we will do our best to make this more secure in the end after the basic functionality works, but there seem to be a couple of misunderstandings here.

It's now using OCP\Security\IHasher

leonklingele-work · 2017-09-19T10:06:37Z

lib/Room.php

 	 */
-	public function enterRoomAsGuest() {
+	public function enterRoomAsGuest($password) {
+		if ($this->getPassword() !== '' && $this->getPassword() !== $password) {


hash_equals

leonklingele-work · 2017-09-19T10:09:38Z

lib/Room.php

@@ -48,6 +49,8 @@ class Room {
 	private $token;
 	/** @var string */
 	private $name;
+	/** @var string */
+	private $password;


Where is this value ever read from the database?

In the manager

codecov · 2017-09-19T14:34:38Z

Codecov Report

Merging #402 into master will decrease coverage by 0.47%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master     #402      +/-   ##
============================================
- Coverage     11.02%   10.55%   -0.48%     
- Complexity      388      406      +18     
============================================
  Files            24       25       +1     
  Lines          1832     1914      +82     
============================================
  Hits            202      202              
- Misses         1630     1712      +82

Impacted Files	Coverage Δ	Complexity Δ
lib/Manager.php	`0% <0%> (ø)`	`56 <1> (ø)`	⬇️
appinfo/routes.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/Controller/CallController.php	`0% <0%> (ø)`	`22 <5> (+8)`	⬆️
lib/Controller/RoomController.php	`0% <0%> (ø)`	`112 <23> (+2)`	⬆️
lib/Migration/Version2001Date20170921145301.php	`0% <0%> (ø)`	`1 <1> (?)`
lib/Room.php	`0% <0%> (ø)`	`55 <13> (+7)`	⬆️

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 07f425d...9257615. Read the comment docs.

nickvergessen · 2017-09-20T13:45:25Z

@Ivansss seems like signaling is going crazy with this:

The thing is, joinCall() returns 403 Forbidden so the user is not joining any call.
But the signaling already starts polling all the time and is kicked out (because there is no session) and restarts...

So signaling should only start on success of InternalSignaling.prototype.joinCall but not on error. any idea where I can find it and how to do it?

nickvergessen · 2017-09-20T14:00:00Z

@fancycode as per above, we are adding room passwords which need to be passed in while joining a room (as guest or internal user which follows a public link and was not added as a participant before)
This should extend the InternalSignaling.prototype.joinCall method to take an additional password argument which is then send to the server.

Please also see the commit above: 14e79c5

can you confirm this will also work on your signaling?

~~(and btw little offtopic, is signaling written with 2 L or 1 L, until now we had 2 everywhere, but your new JS class is single L?)~~ => 39fe6f1

nickvergessen · 2017-09-21T12:45:00Z

Ready for first functional testing

Remaining todos:

decide which information is available for guests without entering the password
Currently participant list is callable without having a valid session (password confirmed)

nickvergessen · 2017-09-21T12:47:02Z

js/signaling.js

+				if (result.status === 403) {
+					// Invalid password
+					OC.dialogs.prompt(
+						t('spreed', 'Please enter the password for this call'),


Actually it would be nice to have the room object in here so we can show the name, if that is okay to leak...

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen · 2017-09-22T14:17:09Z

Ready for testing now

Ivansss

Tested and works 👍

nickvergessen added 2. developing enhancement feature: api 🛠️ OCS API for conversations, chats and participants high labels Sep 11, 2017

nickvergessen added this to the 3.0 (Nextcloud 13) milestone Sep 11, 2017

leonklingele-work reviewed Sep 19, 2017

View reviewed changes

nickvergessen force-pushed the room-password branch from 7945656 to d815a2a Compare September 19, 2017 14:08

nickvergessen force-pushed the room-password branch from 11cf133 to 120a618 Compare September 20, 2017 12:28

nickvergessen force-pushed the room-password branch from db8ca2d to 90dc934 Compare September 20, 2017 14:03

nickvergessen requested a review from Ivansss September 21, 2017 12:45

nickvergessen commented Sep 21, 2017

View reviewed changes

nickvergessen force-pushed the room-password branch 2 times, most recently from 1856a25 to a929fd6 Compare September 22, 2017 13:39

nickvergessen added 13 commits September 22, 2017 16:16

Start implementing room passwords

dcd8210

Signed-off-by: Joas Schilling <coding@schilljs.com>

Add endpoint to set the password

504acc5

Signed-off-by: Joas Schilling <coding@schilljs.com>

Add docs about the new endpoint

834a13c

Signed-off-by: Joas Schilling <coding@schilljs.com>

Only do signaling while we ping (after successful join)

411ff3d

Signed-off-by: Joas Schilling <coding@schilljs.com>

Increase version to run migrations

eb7546e

Signed-off-by: Joas Schilling <coding@schilljs.com>

Add option for the moderator to set a password

f8a9f12

Signed-off-by: Joas Schilling <coding@schilljs.com>

Hash the passwords

1eb766e

Signed-off-by: Joas Schilling <coding@schilljs.com>

Doc paassword on "join call"

6117e5a

Signed-off-by: Joas Schilling <coding@schilljs.com>

Request the password when joining a call with password

aee0e22

Signed-off-by: Joas Schilling <coding@schilljs.com>

Remove empty methods

8d78076

Signed-off-by: Joas Schilling <coding@schilljs.com>

Only return limited information before password verification

ff6bddb

Signed-off-by: Joas Schilling <coding@schilljs.com>

Allow accessing the peers for users which are part of the room

ff26dc0

Signed-off-by: Joas Schilling <coding@schilljs.com>

Remove self-joined users from the list after leaving

5ba66c0

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen added 6 commits September 22, 2017 16:16

ES lint

1233236

Signed-off-by: Joas Schilling <coding@schilljs.com>

Add integration tests for setting the password and joining without it

87bfc88

Signed-off-by: Joas Schilling <coding@schilljs.com>

Yeah... of course the password must be wrong to fail, not correct...

fc2e864

Signed-off-by: Joas Schilling <coding@schilljs.com>

Make sure join with password works :)

c8354fb

Signed-off-by: Joas Schilling <coding@schilljs.com>

Add integration tests for call handling with passwords

e0613c7

Signed-off-by: Joas Schilling <coding@schilljs.com>

Fail when the user is not a participant

9257615

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen force-pushed the room-password branch from a929fd6 to 9257615 Compare September 22, 2017 14:16

nickvergessen added 3. to review and removed 2. developing labels Sep 22, 2017

Ivansss approved these changes Sep 25, 2017

View reviewed changes

Ivansss merged commit 734900b into master Sep 25, 2017

Ivansss deleted the room-password branch September 25, 2017 12:01

nickvergessen mentioned this pull request Sep 25, 2017

Fix integration tests #424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Room passwords #402

Room passwords #402

nickvergessen commented Sep 11, 2017 •

edited

Loading

leonklingele-work Sep 19, 2017

nickvergessen Sep 20, 2017 •

edited

Loading

leonklingele-work Sep 20, 2017

nickvergessen Sep 20, 2017

nickvergessen Sep 21, 2017

leonklingele-work Sep 19, 2017

leonklingele-work Sep 19, 2017

nickvergessen Sep 20, 2017

codecov bot commented Sep 19, 2017 •

edited

Loading

nickvergessen commented Sep 20, 2017

nickvergessen commented Sep 20, 2017 •

edited

Loading

nickvergessen commented Sep 21, 2017

nickvergessen Sep 21, 2017

nickvergessen commented Sep 22, 2017

Ivansss left a comment

Room passwords #402

Room passwords #402

Conversation

nickvergessen commented Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

nickvergessen Sep 20, 2017 • 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

codecov bot commented Sep 19, 2017 • edited Loading

Codecov Report

nickvergessen commented Sep 20, 2017

nickvergessen commented Sep 20, 2017 • edited Loading

nickvergessen commented Sep 21, 2017

Choose a reason for hiding this comment

nickvergessen commented Sep 22, 2017

Ivansss left a comment

Choose a reason for hiding this comment

nickvergessen commented Sep 11, 2017 •

edited

Loading

nickvergessen Sep 20, 2017 •

edited

Loading

codecov bot commented Sep 19, 2017 •

edited

Loading

nickvergessen commented Sep 20, 2017 •

edited

Loading