Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX correctly redirect after successful log in #257

Merged

Conversation

NightJar
Copy link
Contributor

@NightJar NightJar commented Jul 9, 2019

The usage of the BackURL key in the request does not work with MFA as it operates over many reqeusts, interrupting the 'normal' operation (default member authenticator logic).
MFA gets around this by storing the BackURL value in the session tied to the request - but this was being (correctly) cleared before the redirect is performed after a successful log in.
This commit fixes this by making a short lived store in memory for this value, and is still able to be utilised by the redirect logic from the 'normal' operation.

Resolves #250

The usage of the BackURL key in the request does not work with MFA as it operates over many reqeusts, interrupting the 'normal' operation (default member authenticator logic).
MFA gets around this by storing the BackURL value in the session tied to the request - but this was being (correctly) cleared before the redirect is performed after a successful log in.
This commit fixes this by making a short lived store in memory for this value, and is still able to be utilised by the redirect logic from the 'normal' operation.
@NightJar
Copy link
Contributor Author

NightJar commented Jul 9, 2019

I don't know how to test this :/
There are too many moving parts when it comes to the BackURL - I can't mock the parent class that is used to call getBackURL - and to correctly test this logic it requires the redirectAfterSuccessfulLogin method to be called first, in order to clear the session and store the URL in the member field for the class.

I opted for the path of least resistance and just made the PR, if anyone has suggestions I'm happy to learn some tricks :)

@brynwhyman
Copy link

Given that this bug effectively blocks users from logging in, I'm VERY keen to see/help this PR be tested before it's merged. Log in redirects have been causing a number of issues and regressions recently so it's worth the effort reviewing and testing thoroughly before merge.

Copy link
Contributor

@ScopeyNZ ScopeyNZ left a comment

Choose a reason for hiding this comment

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

I'm not really a fan on introducing a property to manage this - as it's only relevant for one flow of this "controller". Information like this should be saved on the request. In fact, I was pretty sure I did this in the past - so I went searching...

This problem, and the other I mentioned this morning in person with skipping is caused by this PR: #212

I think that PR makes some incorrect changes. The BackURL should be placed on the request when we're ready to delegate back to the parent logic (of determining a back URL to go to). Additionally, the "make sure" check that they've reached this handler without completing registration or being allowed to skip should not have been removed.

I'm going to push to this branch to correct these issues, but we'll have to re-test this issue. I'll also put in some tests to cover these cases in the future.

@ScopeyNZ
Copy link
Contributor

I've pushed a commit with the fix I recommend. I've tested #212 with this too. I'll work on putting some functional tests in now.

@NightJar
Copy link
Contributor Author

You have basically rewritten this @ScopeyNZ - is it ready now?

Copy link
Member

@Cheddam Cheddam left a comment

Choose a reason for hiding this comment

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

One minor change suggested, otherwise I'm happy with this.

Copy link
Contributor Author

@NightJar NightJar left a comment

Choose a reason for hiding this comment

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

I can't approve my own PR, but I can comment saying I approve, just a couple of minor comment adjustments :)

src/Authenticator/LoginHandler.php Outdated Show resolved Hide resolved
src/Authenticator/LoginHandler.php Show resolved Hide resolved
@ScopeyNZ ScopeyNZ dismissed their stale review July 11, 2019 05:04

I fixed my request.

@robbieaverill robbieaverill merged commit cd9b6b2 into silverstripe:4.0 Jul 15, 2019
@robbieaverill robbieaverill deleted the pulls/4.0/backup-backurl branch July 15, 2019 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants