Fix flow #6879

mario · 2017-10-19T15:11:10Z

This fixes the new authentication flow as in:

puts the protocol in the returned server url
puts the proper path in the returned server url

Signed-off-by: Mario Danic <mario@lovelyhq.com>

nickvergessen · 2017-10-19T15:12:34Z

tests/Core/Controller/ClientFlowLoginControllerTest.php

@@ -428,7 +428,7 @@ public function testGeneratePasswordWithPassword() {
 			->method('getServerHost')
 			->willReturn('example.com');

-		$expected = new Http\RedirectResponse('nc://login/server:example.com&user:MyLoginName&password:MyGeneratedToken');
+		$expected = new Http\RedirectResponse('nc://login/http://server:example.com&user:MyLoginName&password:MyGeneratedToken');


'nc://login/server:http:// cough

nickvergessen · 2017-10-19T15:13:28Z

tests/Core/Controller/ClientFlowLoginControllerTest.php


-		$expected = new Http\RedirectResponse('nc://login/server:example.com&user:MyLoginName&password:MyGeneratedToken');
+		$expected = new Http\RedirectResponse('nc://login/http://server:example.com&user:MyLoginName&password:MyGeneratedToken');


Signed-off-by: Mario Danic <mario@lovelyhq.com>

mario · 2017-10-19T15:16:36Z

@nickvergessen fixed

mario · 2017-10-19T15:16:52Z

I tested, it works, but I don't know if tests work xD

nickvergessen · 2017-10-19T15:17:15Z

tests/Core/Controller/ClientFlowLoginControllerTest.php

@@ -161,7 +161,7 @@ public function testShowAuthPickerPageWithOcsHeader() {
 		$this->request
 			->expects($this->once())
 			->method('getServerHost')
-			->willReturn('example.com');
+			->willReturn('http://example.com');


this is cheating? make sure it actually calls getServerProtocol ?!

Hm actually wouldnt it return example.com? Since it's just getServerHost ... Tests need better thinking xD

codecov · 2017-10-19T15:29:40Z

Codecov Report

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

@@            Coverage Diff            @@
##             master    #6879   +/-   ##
=========================================
  Coverage          ?   34.68%           
  Complexity        ?    24299           
=========================================
  Files             ?     1577           
  Lines             ?    92928           
  Branches          ?     1359           
=========================================
  Hits              ?    32235           
  Misses            ?    60693           
  Partials          ?        0

Impacted Files	Coverage Δ	Complexity Δ
core/Controller/ClientFlowLoginController.php	`0% <0%> (ø)`	`22 <0> (?)`

mario · 2017-10-19T15:36:37Z

Can we get this in 12 as well? It's important ...

mario · 2017-10-19T17:06:55Z

@nickvergessen all should be fixed.

rullzer · 2017-10-19T18:13:04Z

Why do we need this?

I think we should only allow the new flow with https.

If that can't be then just first try https and if that fails http.

Also. The user already entered an url with protocol to get to this stage. Can't you just fetch the protocol from the url entered?

mario · 2017-10-19T18:26:58Z

a) I don't want to guess (possible two network queries instead of one)
b) We could use the initially entered protocol, but I'd still prefer to have it returned (I trust the server more than I do the client)
c) This makes it possible to use subfolders (example.com/nextcloud) whereas it wouldn't work with the original flow

Also, while allowing all this only with HTTPS is a noble idea, reality is different :)

rullzer · 2017-10-23T07:52:05Z

core/Controller/ClientFlowLoginController.php

@@ -302,7 +302,8 @@ public function generateAppPassword($stateToken,
 			);
 			$this->session->remove('oauth.state');
 		} else {
-			$redirectUri = 'nc://login/server:' . $this->request->getServerHost() . '&user:' . urlencode($loginName) . '&password:' . urlencode($token);
+			$serverPath = $this->request->getServerProtocol() . "://" . $this->request->getServerHost() . substr($this->request->getRequestUri(), 0, strpos($this->request->getRequestUri(), "/index.php"));


This won't work if you have pretty urls. Since then you don't have index.php

Do you have an alternative suggestion? Even on Android for login flow we always use index.php.

Got an idea. Testing.

Added support for pretty urls.

nickvergessen · 2017-11-01T10:10:30Z

core/Controller/ClientFlowLoginController.php

@@ -302,7 +302,16 @@ public function generateAppPassword($stateToken,
 			);
 			$this->session->remove('oauth.state');
 		} else {
-			$redirectUri = 'nc://login/server:' . $this->request->getServerHost() . '&user:' . urlencode($loginName) . '&password:' . urlencode($token);
+			$serverPostfix = "";


Please use single quotes

nickvergessen · 2017-11-01T10:10:40Z

core/Controller/ClientFlowLoginController.php

+				$serverPostfix = substr($this->request->getRequestUri(), 0, strpos($this->request->getRequestUri(), "/login/flow"));
+			}
+
+			$serverPath = $this->request->getServerProtocol() . "://" . $this->request->getServerHost() . $serverPostfix;


single quotes

nickvergessen · 2017-11-01T10:10:48Z

core/Controller/ClientFlowLoginController.php

+			$serverPostfix = "";
+
+			if (strpos($this->request->getRequestUri(), '/index.php') !== false) {
+				$serverPostfix = substr($this->request->getRequestUri(), 0, strpos($this->request->getRequestUri(), "/index.php"));


same here and below

nickvergessen · 2017-11-01T10:12:16Z

Should we merge this then? Or does it break existing stuff etc?

mario · 2017-11-08T19:48:28Z

@nickvergessen updated.

MorrisJobke · 2017-11-08T23:02:16Z

Conflict :/

mario added 2 commits October 19, 2017 17:00

Try

b19f7b3

Signed-off-by: Mario Danic <mario@lovelyhq.com>

Fix path generation

276126c

Signed-off-by: Mario Danic <mario@lovelyhq.com>

mario requested review from rullzer, nickvergessen and LukasReschke October 19, 2017 15:11

nickvergessen reviewed Oct 19, 2017

View reviewed changes

Fix bugs

9064166

Signed-off-by: Mario Danic <mario@lovelyhq.com>

nickvergessen requested changes Oct 19, 2017

View reviewed changes

rullzer reviewed Oct 23, 2017

View reviewed changes

nickvergessen reviewed Nov 1, 2017

View reviewed changes

nickvergessen added the 3. to review Waiting for reviews label Nov 1, 2017

mario force-pushed the fix-flow branch from aae518f to 9064166 Compare November 8, 2017 23:20

mario closed this Nov 8, 2017

mario deleted the fix-flow branch November 8, 2017 23:23

mario restored the fix-flow branch November 8, 2017 23:24

mario reopened this Nov 8, 2017

mario closed this Nov 8, 2017

mario deleted the fix-flow branch November 8, 2017 23:28

mario mentioned this pull request Nov 8, 2017

Fix flow #7115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flow #6879

Fix flow #6879

mario commented Oct 19, 2017

nickvergessen Oct 19, 2017

nickvergessen Oct 19, 2017

mario commented Oct 19, 2017

mario commented Oct 19, 2017

nickvergessen Oct 19, 2017

mario Oct 19, 2017

mario Oct 19, 2017

codecov bot commented Oct 19, 2017 •

edited

Loading

mario commented Oct 19, 2017

mario commented Oct 19, 2017

rullzer commented Oct 19, 2017

mario commented Oct 19, 2017

rullzer Oct 23, 2017

mario Oct 23, 2017

mario Oct 23, 2017

mario Oct 23, 2017

nickvergessen Nov 1, 2017

nickvergessen Nov 1, 2017

nickvergessen Nov 1, 2017

nickvergessen commented Nov 1, 2017

mario commented Nov 8, 2017

MorrisJobke commented Nov 8, 2017


		$expected = new Http\RedirectResponse('nc://login/server:example.com&user:MyLoginName&password:MyGeneratedToken');
		$expected = new Http\RedirectResponse('nc://login/http://server:example.com&user:MyLoginName&password:MyGeneratedToken');

Fix flow #6879

Fix flow #6879

Conversation

mario commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mario commented Oct 19, 2017

mario commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 19, 2017 • edited Loading

Codecov Report

mario commented Oct 19, 2017

mario commented Oct 19, 2017

rullzer commented Oct 19, 2017

mario commented Oct 19, 2017

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 Nov 1, 2017

mario commented Nov 8, 2017

MorrisJobke commented Nov 8, 2017

codecov bot commented Oct 19, 2017 •

edited

Loading