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

Override the authentication listener to validate FORM_SUBMIT #1118

Merged
merged 11 commits into from
Jan 13, 2020

Conversation

aschempp
Copy link
Member

@richardhj can you see if this fixes #558 ?

@aschempp aschempp added this to the 4.9 milestone Dec 19, 2019
@aschempp aschempp self-assigned this Dec 19, 2019
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Concept looks logical to me. Needs tests once @richardhj provided feedback.

Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

The unit tests are still failing and there should be a test for the contao.security.authentication_listener service in the ContaoCoreExtensionTest class.

@leofeyer leofeyer added the bug label Dec 20, 2019
@aschempp aschempp changed the title Override the authentication listener to validate FORM_SUBMIT [WIP] Override the authentication listener to validate FORM_SUBMIT Jan 10, 2020
@aschempp
Copy link
Member Author

aschempp commented Jan 10, 2020

I have update the basic PR features according to my findings yesterday with @richardhj

The current frontend login implementation works around several issues Contao core has with Symfony security. Symfony assumes there's a URL where you POST the login data, and that there is a page where the login form is. By default, Symfony will redirect a user to the login_path, and only check credentials on the check_path. For that case, we created the _contao/login route, and by setting use_forward the system didn't actually redirect but return a subrequest response of that page.

All of this is not actually how Contao works. In the Contao front end, one can place the login module on any page, and on that page the credentials should be checked. We also do not know a login redirect URL, we merely render the 401/403 page type in place.

This is what this PR finally implements now: instead of using the default authentication entry point, which redirects or internally forwards to a login_path, we simply generate the 401 content in that place. Instead of only checking the authentication on the check_path, we override the authentication listener to check whenever a POST request with FORM_SUBMIT=tl_login is sent. We therefore do not need the contao_frontend_login route anymore 🎉

This PR will collide with #1164 and I will rebase and fix issues and tests once #1164 is merged.

richardhj
richardhj previously approved these changes Jan 10, 2020
Copy link
Member

@richardhj richardhj left a comment

Choose a reason for hiding this comment

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

Very good! This fixes #558 and #966. Sadly #967 persists but I will evaluate once #1164 and this PR is merged.

# Conflicts:
#	core-bundle/src/Resources/config/services.yml
#	core-bundle/src/Security/Authentication/AuthenticationEntryPoint.php
@aschempp
Copy link
Member Author

This PR is feature-complete now, but I need to fix tests etc. Basically, we're moving away from configuring Symfony from behaving as needed by Contao, and simply implement the necessary 20% ourselves.

To summarize the changes and make them more "readable":

  1. I have added a custom authentication listener (configured through the security factory). The listener will check for $_POST['FORM_SUBMIT'] === 'tl_login' instead of a check path. This makes check_path obsolete.

  2. As mentioned before, our frontend login route (/_contao/login) has become obsolete thanks to generating the 401 page directly in our security entry point. This makes login_path and use_forward as well as the contao_frontend_login route obsolete.

  3. Thanks to no.2 we do no longer need a failure_path, because that's the place the login form is on. That's now always the current URL, and I adjusted and simplified our AuthenticationFailureHandler accordingly.

  4. By handling base64 directly in the AuthenticationSuccessHandler, we do not need the Base64RedirectController anymore. As we know Contao always uses the _target_path, we don't need configurations for default_target_path.

  5. The authentication success and failure handlers are now configured by the security factory, so the success_handler and failure_handler are no longer needed.

This will collide with #1130 so @bytehead needs to rebase (mostly because I already removed the lock_period etc. config from the security factory.

Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Oh wow, this makes it so much easier for me to understand what's actually going on in the security authentication process 😄 Looks correct to me!

Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

PR looks very good. 👍 The tests are failing though.

core-bundle/src/Resources/config/services.yml Show resolved Hide resolved
@aschempp aschempp changed the title [WIP] Override the authentication listener to validate FORM_SUBMIT Override the authentication listener to validate FORM_SUBMIT Jan 12, 2020
Toflar
Toflar previously approved these changes Jan 12, 2020
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

LGTM

richardhj
richardhj previously approved these changes Jan 13, 2020
@leofeyer leofeyer dismissed stale reviews from richardhj and Toflar via 48a8506 January 13, 2020 08:50
@leofeyer leofeyer merged commit e4e0799 into master Jan 13, 2020
@leofeyer leofeyer deleted the feature/login-form branch January 13, 2020 08:53
@leofeyer
Copy link
Member

Thank you @aschempp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useRawRequestData leads to empty data
4 participants