Skip to content

redirect to default app after solving the 2FA challenge - #1188

Merged
LukasReschke merged 2 commits into
masterfrom
2fa-redirect-default-app
Aug 31, 2016
Merged

redirect to default app after solving the 2FA challenge#1188
LukasReschke merged 2 commits into
masterfrom
2fa-redirect-default-app

Conversation

@ChristophWurst

@ChristophWurstChristophWurst commented Aug 30, 2016

Copy link
Copy Markdown
Member

@ChristophWurstChristophWurst added this to the Nextcloud 11.0 milestone Aug 30, 2016
@mention-bot

mention-bot commented Aug 30, 2016

Copy link
Copy Markdown

@ChristophWurst, thanks for your PR! By analyzing the annotation information on this pull request, we identified @nickvergessen and @rullzer to be potential reviewers

@ChristophWurstChristophWurst mentioned this pull request Aug 30, 2016
Closed
ChristophWurst
ChristophWurst reviewed Aug 30, 2016
View reviewed changes
Comment threadtests/Core/Controller/TwoFactorChallengeControllerTest.php Outdated

$expected = new \OCP\AppFramework\Http\RedirectResponse('files/index/url');
$this->assertEquals($expected, $this->controller->solveChallenge('myprovider', 'token'));
$this->assertInstanceOf('\OCP\AppFramework\Http\RedirectResponse', $this->controller->solveChallenge('myprovider', 'token'));

@ChristophWurstChristophWurstAug 30, 2016

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

is there a way to reliably determine the output of OC_Util::getDefaultPageUrl() while testing? Then I could compare the actual return values here.

@LukasReschkeLukasReschkeAug 30, 2016

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.

I don't think so, but you can just make $expected use OC_Util::getDefaultPageUrl() as well. Not perfect, but works ;)

@rullzer

rullzer commented Aug 31, 2016

Copy link
Copy Markdown
Member

LGTM

1 similar comment
@LukasReschke

LukasReschke commented Aug 31, 2016

Copy link
Copy Markdown
Member

LGTM

@LukasReschke
LukasReschke merged commit 2685129 into masterAug 31, 2016
@LukasReschke
LukasReschke deleted the 2fa-redirect-default-app branch August 31, 2016 17:46
@juliusknorr

juliusknorr commented Sep 15, 2016•
edited
Loading

Copy link
Copy Markdown
Member

This is a bug fix, so i guess we should backport this? cc @karlitschek@ChristophWurst

Bug on stable9 stable10 reported on the forums: https://help.nextcloud.com/t/apporder-and-totp-wrong-landing-page/3252

@nickvergessen

nickvergessen commented Sep 15, 2016

Copy link
Copy Markdown
Member

stable9 doesnt have 2 factor, therefor the fix is different.
But was this backported to 10? @ChristophWurst

@ChristophWurst

ChristophWurst commented Sep 15, 2016

Copy link
Copy Markdown
MemberAuthor

I don't think this was backported, or at least I didn't do it

@juliusknorr

juliusknorr commented Sep 15, 2016

Copy link
Copy Markdown
Member

@nickvergessen Ah of course, i mean stable10

@ghost

ghost commented Sep 15, 2016•
edited by ghost
Loading

Copy link
Copy Markdown

@ChristophWurst@juliushaertl@nickvergessen Sorry, this was my fault. I posted a wrong version number in the forum. The correct version is 9.1.0.16.

It works. Tank you!

@karlitschek

karlitschek commented Sep 15, 2016

Copy link
Copy Markdown
Member

I think a backport makes sense! 👍

@juliusknorrjuliusknorr mentioned this pull request Sep 21, 2016
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

3. to reviewWaiting for reviewsenhancement

Projects

None yet

Milestone

Nextcloud 11.0

Development

Successfully merging this pull request may close these issues.

Redirect after login with TOTP

7 participants

@ChristophWurst@mention-bot@rullzer@LukasReschke@juliusknorr@nickvergessen@karlitschek