Added hook to verify password to allow custom password check #1020

Peterede · 2018-07-04T02:13:01Z

I have added a hook to allow overriding the password check in order to provide a different password evaluation. I am using this so that each user can have a different password.

fancycode

Please make sure to sign-off your commits as described here to fix the CI errors:
https://github.com/nextcloud/server/blob/master/CONTRIBUTING.md#sign-your-work

Also the new functionality should have a test to avoid breaking it in the future.

fancycode · 2018-07-06T07:33:01Z

lib/Room.php

+		$event = new GenericEvent(null, [
+			'password' => $password,
+			'hasPassword' => $this->hasPassword(),
+		]);


You should pass the room as subject of the event, so the event handler knows which room is checking the password and has access to all parameters:

new GenericEvent($this, [ 'password' => $password, ]);

Peterede · 2018-07-09T00:35:56Z

I am not sure on how to create the test, should it be something like:

	$this->dispatcher->expects($this->at(0))
		->method('dispatch')
		->with('\OCA\Spreed\Room::verifyPassword');

fancycode

The test should register an event handler that performs the custom validation. That way, the evaluation of the handler result in talk is tested. Your sample code would only ensure that the event is triggered, but not that its result is evaluated.

For the CI to pass, you need to sign-off all previous commits in this PR (using git rebase -i) or squash all commits into one (signed-off) commit.

fancycode · 2018-07-09T07:19:39Z

lib/Room.php

-
+		$event = new GenericEvent($this, [
+				'password' => $password,
+				'hasPassword' => $this->hasPassword(),


The hasPassword shouldn't be included in the event, the event handler can use the subject of the event (i.e. the room) to call hasPassword if necessary.

fancycode · 2018-07-09T07:19:49Z

lib/Room.php

@@ -633,6 +649,15 @@ public function changeInCall($sessionId, $active) {
 	 * @return bool
 	 */
 	public function verifyPassword($password) {
+		$event = new GenericEvent($this, [
+			'password' => $password,
+			'hasPassword' => $this->hasPassword(),


fancycode · 2018-07-09T07:20:48Z

lib/Room.php

+	/**
+	 * @param int $length
+	 */
+	public function generateSessionId($length) {


Where is this used? Please remove if it doesn't belong to this PR.

This is used in the hook preJoinRoomGuest to generate a sessionId and return it from the event. secureRandom is private in the Room class so this function is needed for this purpose. This is part of my work on allowing guests to be assigned moderator status so in preJoinRoomGuest I create a sessionId and update the database for the user based upon email address and return the session.

I will remove it for this PR

… Peter Edens <petere@conceiva.com>

…ceiva.com>

Peterede · 2018-07-12T04:00:36Z

I have added tests for verifying the password and testing the dispatch of the event

nickvergessen · 2018-07-17T09:30:53Z

lib/Room.php

+		$this->dispatcher->dispatch(self::class . '::verifyPassword', $event);
+		if ($event->hasArgument('result')) {
+			$result = $event->getArgument('result');
+			return $result;


I would prefer:

return [ 'result' => $result['result'] ?? false, 'url' => $result['url'] ?? '' ];

here, just to make sure that the values we require in our code are returned

nickvergessen · 2018-07-17T09:31:49Z

lib/Controller/PageController.php

@@ -147,10 +147,22 @@ public function index($token = '', $callUser = '', $password = '') {
 				}

 				if ($requirePassword) {
-					if ($password !== '' && $room->verifyPassword($password)) {
+					$passwordVerification = [


Useless assignment

nickvergessen · 2018-07-17T09:32:09Z

lib/Controller/PageController.php

 						$this->session->setPasswordForRoom($token, $token);
 					} else {
-						return new TemplateResponse($this->appName, 'authenticate', [], 'guest');
+						if ($passwordVerification['url'] == '') {


empty($passwordVerification['url']) or === ''

nickvergessen · 2018-07-17T09:32:22Z

appinfo/info.xml

@@ -44,7 +44,7 @@ And in the works for the [coming versions](https://github.com/nextcloud/spreed/m
 	<screenshot>https://github.com/raw/nextcloud/spreed/master/docs/contacts-menu.png</screenshot>

 	<dependencies>
-		<nextcloud min-version="14" max-version="14" />
+		<nextcloud min-version="13" max-version="14" />


Revert this please

…ceiva.com>

nickvergessen · 2018-07-18T11:17:06Z

lib/Controller/PageController.php

@@ -194,10 +202,22 @@ protected function guestEnterRoom($token, $password) {

 		$this->session->removePasswordForRoom($token);
 		if ($room->hasPassword()) {
-			if ($password !== '' && $room->verifyPassword($password)) {
+			$passwordVerification = [


useless assignment

nickvergessen · 2018-07-18T11:17:17Z

lib/Controller/PageController.php

 				$this->session->setPasswordForRoom($token, $token);
 			} else {
-				return new TemplateResponse($this->appName, 'authenticate', [], 'guest');
+				if ($passwordVerification['url'] == '') {


nickvergessen · 2018-07-18T11:18:53Z

tests/php/PasswordVerificationTest.php

+	public function testVerifyPassword() {
+		$dispatcher = \OC::$server->getEventDispatcher();
+
+		$dispatcher->addListener('OCA\Spreed\Room::verifyPassword', function(GenericEvent $event) {


missing import of Symfony\Component\EventDispatcher\GenericEvent

OCA\Spreed\Tests\php\Signaling\BackendNotifierTest::testRoomInCallChanged
TypeError: Argument 1 passed to OCA\Spreed\Tests\php\PasswordVerificationTest::OCA\Spreed\Tests\php{closure}() must be an instance of OCA\Spreed\Tests\php\GenericEvent, instance of Symfony\Component\EventDispatcher\GenericEvent given, called in /drone/src/github.com/nextcloud/server/3rdparty/symfony/event-dispatcher/EventDispatcher.php on line 212

/drone/src/github.com/nextcloud/server/apps/spreed/tests/php/PasswordVerificationTest.php:35
/drone/src/github.com/nextcloud/server/3rdparty/symfony/event-dispatcher/EventDispatcher.php:212
/drone/src/github.com/nextcloud/server/3rdparty/symfony/event-dispatcher/EventDispatcher.php:44
/drone/src/github.com/nextcloud/server/apps/spreed/lib/Room.php:645
/drone/src/github.com/nextcloud/server/apps/spreed/lib/Room.php:583
/drone/src/github.com/nextcloud/server/apps/spreed/tests/php/Signaling/BackendNotifierTest.php:287

nickvergessen · 2018-07-18T11:19:47Z

tests/php/PasswordVerificationTest.php

+use OCP\Security\ISecureRandom;
+use Test\TestCase;
+
+class PasswordVerificationTest extends TestCase {


missing database group:

/** * @group DB */

nickvergessen · 2018-07-20T12:15:14Z

Thanks @Peterede I fixed the remaining issues in a follow up PR: #1066

nickvergessen · 2018-09-19T15:15:24Z

And #1077

Peterede mentioned this pull request Jul 5, 2018

Spreed integration nextcloud/calendar#563

Closed

10 tasks

fancycode requested changes Jul 6, 2018

View reviewed changes

nickvergessen added 2. developing enhancement feature: api 🛠️ OCS API for conversations, chats and participants labels Jul 6, 2018

nickvergessen added this to the 4.0 (Nextcloud 14) milestone Jul 6, 2018

nickvergessen self-requested a review July 6, 2018 08:32

nickvergessen added the backport-request label Jul 6, 2018

fancycode requested changes Jul 9, 2018

View reviewed changes

Returning result and redirect url from verifyPassword. Signed-off-by:…

12e8250

… Peter Edens <petere@conceiva.com>

Peterede force-pushed the verify-password-hook branch from e6f46fc to 12e8250 Compare July 12, 2018 03:23

Added test for verifyPassword. Signed-off-by: Peter Edens <petere@con…

a0bd7af

…ceiva.com>

nickvergessen reviewed Jul 17, 2018

View reviewed changes

nickvergessen requested changes Jul 17, 2018

View reviewed changes

Requested changes from review. Signed-off-by: Peter Edens <petere@con…

09b0c01

…ceiva.com>

nickvergessen requested changes Jul 18, 2018

View reviewed changes

added missing import. Signed-off-by: Peter Edens <petere@conceiva.com>

54d3862

nickvergessen merged commit f482145 into nextcloud:master Jul 20, 2018

nickvergessen mentioned this pull request Jul 20, 2018

Fix some follow up issues with 1020 #1066

Merged

nickvergessen removed 2. developing backport-request labels Sep 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added hook to verify password to allow custom password check #1020

Added hook to verify password to allow custom password check #1020

Peterede commented Jul 4, 2018

fancycode left a comment

fancycode Jul 6, 2018

Peterede commented Jul 9, 2018

fancycode left a comment

fancycode Jul 9, 2018

fancycode Jul 9, 2018

fancycode Jul 9, 2018

Peterede Jul 10, 2018

Peterede Jul 10, 2018

Peterede commented Jul 12, 2018

nickvergessen Jul 17, 2018

nickvergessen Jul 17, 2018

nickvergessen Jul 17, 2018

nickvergessen Jul 17, 2018

nickvergessen Jul 18, 2018

nickvergessen Jul 18, 2018

nickvergessen Jul 18, 2018

nickvergessen Jul 18, 2018

nickvergessen commented Jul 20, 2018

nickvergessen commented Sep 19, 2018

Added hook to verify password to allow custom password check #1020

Added hook to verify password to allow custom password check #1020

Conversation

Peterede commented Jul 4, 2018

fancycode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peterede commented Jul 9, 2018

fancycode 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peterede commented Jul 12, 2018

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

nickvergessen commented Jul 20, 2018

nickvergessen commented Sep 19, 2018