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

Two-factor front end listener incomplete #567

Closed
leofeyer opened this issue Jul 9, 2019 · 17 comments

Comments

Projects
None yet
3 participants
@leofeyer
Copy link
Member

commented Jul 9, 2019

There are two issues with the TwoFactorFrontendListener class:

if (!$unauthorizedPage->redirect) {

  1. $unauthorizedPage->redirect is either permanent or temporary. @bytehead I assume you meant autoforward?

  2. If 2FA has been set up and the 401 page has an auto-forward page, the listener will endlessly redirect me to the login screen. This is because it does not handle the case "token fully authenticated".

@leofeyer leofeyer added the defect label Jul 9, 2019

@leofeyer leofeyer added this to the 4.8 milestone Jul 9, 2019

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

This also seems wrong:

if (!$unauthorizedPage->redirect) {
return;
}

If the 401 page does not forward to another page, we return and bypass the 2FA check. Is this intended?

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

@bytehead Can you elaborate on what exactly the TwoFactorFrontendListener listener is supposed to do?

@bytehead

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Afaik this is a change from @aschempp - isn't it?

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

And there is an unresolved comment from you regarding these lines:

if (!$token instanceof TwoFactorToken && !\in_array(\get_class($token), $this->supportedTokens, true)) {
return;
}

This is wrong. We only need to check for TwoFactorToken. It's actually a TwoFactorTokenListener so everything else does not matter.

This configuration parameter from the two-factor-bundle means something different:

    # The security token classes, which trigger two-factor authentication.
    # By default the bundle only reacts to Symfony's username+password authentication. If you want to enable
    # two-factor authentication for other authentication methods, add their security token classes.
    security_tokens:
        - Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken

https://github.com/scheb/two-factor-bundle/blob/master/Resources/doc/configuration.md

These possibly other tokens get wrapped into a TwoFactorToken anyway.

What about it?

@bytehead

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

You can ignore this comment, I didn't get the intended behaviour of @aschempp's change of this.

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

Afaik this is a change from @aschempp - isn't it?

It seems so: 11e2d7d

I cannot judge if the commit needs to be reverted, though, unless I know what exactly the TwoFactorFrontendListener listener is supposed to do. Can you explain?

@bytehead

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

as far as I can see - it possibly should be autoforward (makes more sense).

@aschempp

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Sorry to say, but the breaking changes are all from @bytehead. I think this one is the problem: 884db46#diff-cc3ec0911c6de9447bc74fb5e984923fL113

@aschempp

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

but it sure should have been autoforward, that's my mistake 🙃

@bytehead

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

My change just gets rid of the superfluous else.

@aschempp

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

I cannot judge if the commit needs to be reverted, though, unless I know what exactly the TwoFactorFrontendListener listener is supposed to do. Can you explain?

The listener does two things:

  1. If you are logged in, 2FA is enforced but not configured for your user, it will force redirect you to the 2FA setup page
  2. If you are in the 2FA authentication process (username+pw valid but 2FA not given), it will make sure you enter the code and don't navigate away. That's done with two approaches
    • if there is a 401 page, we assume there must be a login module on it (which will show the 2FA process). If the 401 page has a forward page, we also assume that page has a login module and is valid. In any of these cases, we throw an access denied exception, which will be catched by our exception handler and show the 401 page.
    • if no 401 is given, we assume the page where the user logged in from must be valid
@aschempp

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

My change just gets rid of the superfluous else.

yeah, but now the second process is also taken into account if the user has a UsernamePasswordToken

@bytehead

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

True that. 🙈

leofeyer added a commit that referenced this issue Jul 9, 2019

Fix endless redirects if 2FA is enabled (see #568)
Description
-----------

See #567

Commits
-------

5846b90 Fix endless redirect
9e0338d Remove useless check
aaeeeb5 Add test
fa25be7 Clean up
a2f3888 Use autoforward property instead of redirect
94fe855 Fix phpstan
@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

So this is still a major issue:

if ($unauthorizedPage instanceof PageModel) {
if (!$unauthorizedPage->autoforward) {
return;
}

If the error 401 page does not have the "autoforward" option enabled, the 2FA check is bypassed. Here is how you can test it:

  1. Use the COD with an error 401 page that has an autofoward page.
  2. Set up 2FA for a member.
  3. Log out and log back in, but stop after you have entered the password.
  4. Try to navigate the website – you should be stuck on the login page.

Now disable the autoforward option on the error 401 page and try to navigate the website. It is now possible to go anywhere.

@bytehead

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Confirmed, I'm on it.

@bytehead

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

See #570.

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

Thanks. ❤️

leofeyer added a commit that referenced this issue Jul 10, 2019

Fix redirect behaviour without 401 autoforward page (see #570)
Description
-----------

Redirect to target path, if there is no 401 page with autoforward enabled.
See #567.

Commits
-------

b971f1f Fix redirect behaviour without 401 autoforward page

@leofeyer leofeyer closed this Jul 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.