Skip to content

Fix flow - #6879

Closed
mario wants to merge 3 commits into
masterfrom
fix-flow
Closed

Fix flow#6879
mario wants to merge 3 commits into
masterfrom
fix-flow

Conversation

@mario

@mariomario commented Oct 19, 2017

Copy link
Copy Markdown
Contributor

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

@mario
Try
b19f7b3
Signed-off-by: Mario Danic <mario@lovelyhq.com>
@mario
Fix path generation
276126c
Signed-off-by: Mario Danic <mario@lovelyhq.com>
nickvergessen
nickvergessen reviewed Oct 19, 2017
View reviewed changes
Comment threadtests/Core/Controller/ClientFlowLoginControllerTest.php Outdated
->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');

@nickvergessennickvergessenOct 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

nickvergessen
nickvergessen reviewed Oct 19, 2017
View reviewed changes
Comment threadtests/Core/Controller/ClientFlowLoginControllerTest.php Outdated
->willReturn('http://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');

@nickvergessennickvergessenOct 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

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

mario commented Oct 19, 2017

Copy link
Copy Markdown
ContributorAuthor

@nickvergessen fixed

@mario

mario commented Oct 19, 2017

Copy link
Copy Markdown
ContributorAuthor

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

nickvergessen
nickvergessen requested changes Oct 19, 2017
View reviewed changes
Comment threadtests/Core/Controller/ClientFlowLoginControllerTest.php
->expects($this->once())
->method('getServerHost')
->willReturn('example.com');
->willReturn('http://example.com');

@nickvergessennickvergessenOct 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mariomarioOct 19, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mariomarioOct 19, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@codecov

codecovBot commented Oct 19, 2017•
edited
Loading

Copy link
Copy Markdown

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 FilesCoverage ΔComplexity Δ
core/Controller/ClientFlowLoginController.php0% <0%> (ø)22 <0> (?)

@mario

mario commented Oct 19, 2017

Copy link
Copy Markdown
ContributorAuthor

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

@mario

mario commented Oct 19, 2017

Copy link
Copy Markdown
ContributorAuthor

@nickvergessen all should be fixed.

@rullzer

rullzer commented Oct 19, 2017

Copy link
Copy Markdown
Member

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

mario commented Oct 19, 2017

Copy link
Copy Markdown
ContributorAuthor

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
rullzer reviewed Oct 23, 2017
View reviewed changes
Comment threadcore/Controller/ClientFlowLoginController.php
$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"));

@rullzerrullzerOct 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mariomarioOct 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mariomarioOct 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got an idea. Testing.

@mariomarioOct 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added support for pretty urls.

nickvergessen
nickvergessen reviewed Nov 1, 2017
View reviewed changes
Comment threadcore/Controller/ClientFlowLoginController.php Outdated
$this->session->remove('oauth.state');
} else {
$redirectUri = 'nc://login/server:' . $this->request->getServerHost() . '&user:' . urlencode($loginName) . '&password:' . urlencode($token);
$serverPostfix = "";

@nickvergessennickvergessenNov 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use single quotes

nickvergessen
nickvergessen reviewed Nov 1, 2017
View reviewed changes
Comment threadcore/Controller/ClientFlowLoginController.php Outdated
$serverPostfix = substr($this->request->getRequestUri(), 0, strpos($this->request->getRequestUri(), "/login/flow"));
}

$serverPath = $this->request->getServerProtocol() . "://" . $this->request->getServerHost() . $serverPostfix;

@nickvergessennickvergessenNov 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single quotes

nickvergessen
nickvergessen reviewed Nov 1, 2017
View reviewed changes
Comment threadcore/Controller/ClientFlowLoginController.php Outdated
$serverPostfix = "";

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

@nickvergessennickvergessenNov 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here and below

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

nickvergessen commented Nov 1, 2017

Copy link
Copy Markdown
Member

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

@mario

mario commented Nov 8, 2017

Copy link
Copy Markdown
ContributorAuthor

@nickvergessen updated.

@MorrisJobke

MorrisJobke commented Nov 8, 2017

Copy link
Copy Markdown
Member

Conflict :/

@mariomario closed this Nov 8, 2017
@mario
mario deleted the fix-flow branch November 8, 2017 23:23
@mario
mario restored the fix-flow branch November 8, 2017 23:24
@mariomario reopened this Nov 8, 2017
@mariomario closed this Nov 8, 2017
@mario
mario deleted the fix-flow branch November 8, 2017 23:28
@mariomario mentioned this pull request Nov 8, 2017
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rullzerrullzerrullzer left review comments
@nickvergessennickvergessennickvergessen requested changes
@LukasReschkeLukasReschkeAwaiting requested review from LukasReschke

Assignees

No one assigned

Labels

3. to reviewWaiting for reviews

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mario@rullzer@nickvergessen@MorrisJobke