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

[BUG] Fix issues with multiple authenticators for a single task #7149

Merged

Conversation

Firesphere
Copy link
Contributor

Using multiple 2FA authenticators, logging out, resetting password etc. proved to be handled wrong.
Example scenario:

Person logs out while multiple authenticators support logout

The result is an error, because the renderWrappedController was called, despite the responses being a set of either array with Content or Form, or a redirect action.

The default action should be followed and not try to render if there is nothing to render

Because the logout (or changepassword, or resetpassword, etc.) has already been handled, the first response is the default authenticator's response. This could be a form (in case of logout without valid token), a content set (reset password) or a form (change password).

This edge case only happens when there are multiple authenticators supporting the requested method that is not login.

@Firesphere Firesphere changed the base branch from master to 4 July 10, 2017 05:28
@Firesphere Firesphere force-pushed the pulls/multiple-logout-authenticators branch 2 times, most recently from ff8654e to fd5a5cc Compare July 10, 2017 05:37
@Firesphere
Copy link
Contributor Author

Explanation:

My test case is based on the usage of @camfindlay his 2FA method side-by-side with my YubiAuth module.
What happens on logout, is the request is delegated to the aggregateTabbed, which will fail, because both modules will respond with an HTTPResponse.

The reason for failure, is that both Authenticators support logout (or reset password, or what else), and therefore, will respond with a valid HTTPResponse the Security::handleMultiple doesn't support.

This PR resolves the handle multiple, by returning the first successful response. This works, because the user at this point is already logged out, so there's no further need for confirmation. With the exception of a user with an expired logout token. In which case, the confirmation screen by @kinglozzer is shown.

Using multiple 2FA authenticators, logging out, resetting password etc. proved to be handled wrong.
Example scenario:
The result is an error, because the `renderWrappedController` was called, despite the responses being a set of either array with Content or Form, or a redirect action.

The default action should be followed and not try to render if there is nothing to render

Because the logout (or changepassword, or resetpassword, etc.) has already been handled, the first response is the default authenticator's response. This _could_ be a form (in case of logout without valid token), a content set (reset password) or a form (change password).

This edge case only happens when there are multiple authenticators supporting the requested method that is _not_ login.
@Firesphere Firesphere force-pushed the pulls/multiple-logout-authenticators branch from fd5a5cc to 2790fa8 Compare July 10, 2017 06:57
@Firesphere
Copy link
Contributor Author

No new tests, as there is no new scenario to test. Happy to add if requested!

@tractorcow tractorcow merged commit 3e97b99 into silverstripe:4 Jul 13, 2017
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.

None yet

2 participants