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

[RTM] Use Symfony Security for authentication purposes in Contao #685

Merged
merged 170 commits into from Dec 12, 2017

Conversation

@bytehead
Member

bytehead commented Jan 24, 2017

This is a working draft to replace the current simple_preauth mechanism to login backend users and frontend members with the form_login from the Symfony Security Component.

Please review and mention all missed topics.

ToDos:

  • Keep redirect on login
  • Implement role switcher / impersonate any BackendUser
  • Session Life Time (framework.session.cookie_lifetime)
  • Rename firewalls (prefix with contao_)
  • Implement Security for FrontendUser
  • Replace postAuthenticate
  • Replace postLogin
  • Replace checkCredentials (needs a custom AuthenticationProvider)
  • Replace importUser
  • Replace postLogout
  • Replace current accountLocked stuff
  • Test Frontend Preview
  • Write tests
  • Check Code style
  • Deprecate all old hooks
  • Provide Event for legacy postAuthenticate hook
  • Provide Event for legacy checkCredentials hook
  • Provide Event for legacy importUser hook
  • Provide Event for legacy postLogout hook
  • Show backend login errors

Fixes needed:

  • Backend login redirect: Simple test: login, go somewhere, delete your session cookie.
  • UserProvider refreshUser without credentials
  • Provide exception messages
  • Replace usage of static legacy class methods (e.g. Config, Environment)
  • Call importUser only on POST request
  • Additional check with UserChecker in Frontend
  • Implement the EquatableInterface on User to compare the token object and DB object
  • on BackendUser::getInstance check if there is a valid _security_contao_backend token in the actual session to restore the user from
  • on FrontendUser::getInstance check if there is a valid _security_contao_frontend token in the actual session to restore the user from

Nice to have (possibly future PRs):

  • use DoctrineTokenProvider for persistent remember me tokens

Possible new features (future PRs):

  • Allow login also with the email address (not only username)
  • Allow some user action via command like activate, change-password or similar

@bytehead bytehead referenced this pull request Jan 24, 2017

Closed

[WIP] Symfony authentication #559

0 of 7 tasks complete

@leofeyer leofeyer added the feature label Jan 24, 2017

@leofeyer leofeyer added this to the 4.4.0 milestone Jan 24, 2017

@leofeyer leofeyer removed this from the 4.4.0 milestone Jan 25, 2017

@leofeyer leofeyer changed the title from [WIP] Feature/symfony authentication [refactored] to [WIP] Feature/symfony authentication refactored Jan 25, 2017

@bytehead bytehead changed the title from [WIP] Feature/symfony authentication refactored to [RFC] Feature/symfony authentication refactored Jan 25, 2017

@bytehead bytehead changed the title from [RFC] Feature/symfony authentication refactored to [WIP] Feature/symfony authentication refactored Feb 3, 2017

*/
public function switchUser($row, $href, $label, $title, $icon)
{
@trigger_error('Using tl_user->switchUser() has been deprecated and will no longer work in Contao 5.0. Use the switch_user_button_generator service instead', E_USER_DEPRECATED);

This comment has been minimized.

@aschempp

aschempp Apr 25, 2017

Contributor

The rest of this method should be removed or replaces by the new switch user stuff.

This comment has been minimized.

@bytehead
/** @var BackendUser $targetUser */
$targetUser = $event->getTargetUser();
$this->logger->info(sprintf("User %s has switched to user %s.", $user->username, $targetUser->username));

This comment has been minimized.

@aschempp

aschempp Apr 25, 2017

Contributor

The only purpose of this listener is to log the action? If that's a replacement of an existing log, we should log with a ContaoContext to make it appear in the backend log.

This comment has been minimized.

@bytehead

bytehead Apr 25, 2017

Member

That should be the replacement of the existing log, but I didn't get it how it's done to make it appear in the backend.

This comment has been minimized.

@aschempp

aschempp Apr 25, 2017

Contributor

$logger->log($level, $strText, array('contao' => new ContaoContext($strFunction, $strCategory)));

This comment has been minimized.

@bytehead

bytehead Apr 25, 2017

Member

Ah, thank you! I'll update it later.

- "@contao.framework"
contao.security.switch_user_button_generator:
class: Contao\CoreBundle\Security\User\SwitchUserButtonGenerator

This comment has been minimized.

@aschempp

aschempp Apr 25, 2017

Contributor

To me this is an event (callback) listener and should be in the according namespace.

if ($authorizationChecker->isGranted('ROLE_PREVIOUS_ADMIN')) {
$logoutLink = $router->generate('contao_backend', [
'_switch_user' => '_exit',

This comment has been minimized.

@aschempp

aschempp Apr 25, 2017

Contributor

this is a new feature, right? Previously a login just logged out whatever user was currently in. Might be confusing if we don't change the logout label to "switch back to X".

This comment has been minimized.

@bytehead

bytehead Apr 25, 2017

Member

Good point :)

This comment has been minimized.

@bytehead

bytehead Nov 5, 2017

Member

The current label is Close the current session and thus not wrong in my opinion.

This comment has been minimized.

@bytehead

bytehead Dec 8, 2017

Member

Changed in e2bed63.

$stmt->execute();
if (0 === $stmt->rowCount()) {
throw new UserNotFoundException('Invalid user ID' . $row['id']);

This comment has been minimized.

@aschempp

aschempp Apr 25, 2017

Contributor

This should never happen, so I would simply return empty string (remove action) in this case and not add a new exception for a never-happening case :)

'_switch_user' => $user->username
]);
return $this->twig->render('@ContaoCore/Backend/switch_user.html.twig', [

This comment has been minimized.

@aschempp

aschempp Apr 25, 2017

Contributor

Do we need a twig template for this? Generally good, but there are so many places where we currently don't…

This comment has been minimized.

@bytehead

bytehead Apr 25, 2017

Member

Yes IMHO we need exactly that. Otherwise we will end up like the DCA classes are right now with template code in the logic parts. And we have to start using (twig) templates at some point.

This comment has been minimized.

@aschempp

aschempp Apr 25, 2017

Contributor

ok, but then this should be a general button template, right?

This comment has been minimized.

@bytehead

bytehead Apr 25, 2017

Member

Good idea. Should be discussed with the others probably?

@aschempp

This comment has been minimized.

Contributor

aschempp commented on src/EventListener/SwitchUserListener.php in 87840fd Apr 25, 2017

Shoulnt this be ContaoContext:ACCESS ?

This comment has been minimized.

Member

bytehead replied Apr 25, 2017

Correct.

bytehead added some commits Apr 25, 2017

*/
protected function checkIfLoginIsAllowed(User $user): void
{
if ($user instanceof FrontendUser && false === $user->login) {

This comment has been minimized.

@leofeyer

leofeyer Dec 11, 2017

Member

Are you sure that the strict type check works here? $user->login is a char(1) column which is either '' or '1' unless casted to boolean.

This comment has been minimized.

@bytehead

bytehead Dec 11, 2017

Member

Good catch! 👍

- "@logger"
tags:
- { name: kernel.event_listener, event: security.switch_user, method: onSwitchUser }
- { name: monolog.logger, channel: contao }

This comment has been minimized.

@leofeyer

leofeyer Dec 11, 2017

Member

Is this a left-over tag?

This comment has been minimized.

@bytehead

bytehead Dec 11, 2017

Member

Yes, it's removed now.

{
foreach ($GLOBALS['TL_HOOKS']['checkCredentials'] as $callback)
// Return if its not a real login attempt
if ($password === null && !$request->isMethod(Request::METHOD_POST))

This comment has been minimized.

@leofeyer

leofeyer Dec 11, 2017

Member

What's the purpose of this line? What do you mean by "not a real login attempt"?

This comment has been minimized.

@bytehead

bytehead Dec 11, 2017

Member

loadUserByUsername will be called even on refreshUser, but the following logic only needs to be executed on a real login and not on a refresh.

*
* @param DataContainer $dc
*/
public function checkRemoveSession($dc)

This comment has been minimized.

@bytehead

bytehead Dec 12, 2017

Member

No BC break if you remove a public method?

This comment has been minimized.

@leofeyer

leofeyer Dec 12, 2017

Member

It is a BC break for sure. The question is whether out BC promise includes DCA files, which I think they don't.

@@ -1 +0,0 @@
<a href="{{ url }}" title="{{ title }}">{{ image | raw }}</a>

This comment has been minimized.

@bytehead
'title' => $title,
'image' => Image::getHtml($icon, $label),
]);
return sprintf('<a href="%s" title="%s">%s</a>', $url, $title, Image::getHtml($icon, $label));

This comment has been minimized.

@bytehead

bytehead Dec 12, 2017

Member

I'm totally against to remove the template. Now we have again html somewhere in the logic.

@leofeyer

This comment has been minimized.

Member

leofeyer commented Dec 12, 2017

The PR is almost ready to merge. There are two open issues:

  1. Do we want to log impersonations?
  2. Do we need to change any private variables or methods to protected so people can overwrite certain things?

Regarding 1.: We are logging "switch user" actions but not "log in as member" actions. Is this ok or should we log both or none?

@bytehead

This comment has been minimized.

Member

bytehead commented Dec 12, 2017

  1. I would like to have impersonations logged. I'm fine to log frontend impersonations as well.
  2. I'm against to make just everything private. Makes it unnecessary hard to extend existing stuff.
$objTemplate->user = \Input::post('user');
/** @var User $user */
$user = $token->getUser();
$objUser = \MemberModel::findByUsername($user->getUsername());

This comment has been minimized.

@leofeyer

leofeyer Dec 12, 2017

Member

This is also something that makes me wonder. Shouldn't $token->getUser() return the user object already? Why are we loading the member model?

This comment has been minimized.

@bytehead

bytehead Dec 12, 2017

Member

The getUser() on the token can have a fully loaded user instance, but you cannot be sure.
Normally it holds only the unserialized data from the session storage.

@@ -13,16 +13,19 @@
<?php endif; ?>
<input type="hidden" name="FORM_SUBMIT" value="<?= $this->formId ?>">
<input type="hidden" name="REQUEST_TOKEN" value="{{request_token}}">
<?php if ($this->targetPath): ?>
<input type="hidden" name="<?= $this->targetName ?>" value="<?= $this->targetPath ?>">

This comment has been minimized.

@leofeyer

leofeyer Dec 12, 2017

Member

I also wonder if this is prone to manipulation? @ausi /cc

This comment has been minimized.

@bytehead

bytehead Dec 12, 2017

Member

Maybe we should check, if it's an existing page from the installation?

This comment has been minimized.

This comment has been minimized.

@aschempp

aschempp Dec 12, 2017

Contributor

well you can only manipulate it through the web inspector. Which means the redirect target after login is manipulated. Can't really see an issue in that, it does not send any data to that URL.

This comment has been minimized.

@ausi

ausi Dec 12, 2017

Member

htmlspecialchars() is missing for $this->targetPath, either in the template or in ModuleLogin. As it is now it’s vulnerable to XSS.

This comment has been minimized.

@leofeyer

@leofeyer leofeyer merged commit 01a6845 into contao:develop Dec 12, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 97.328%
Details
@leofeyer

This comment has been minimized.

Member

leofeyer commented Dec 12, 2017

Thank you, big time @bytehead.

@bytehead bytehead deleted the bytehead:feature/symfony-authentication-2 branch Mar 6, 2018

richardhj added a commit to richardhj/contao that referenced this pull request Nov 3, 2018

Respect jump to on close account.
Functionality broke with contao/core-bundle#685 as User->logout uses a RedirectResponseException.

richardhj added a commit to richardhj/contao that referenced this pull request Nov 3, 2018

Respect jump to on close account.
Functionality broke with contao/core-bundle#685 as User->logout uses a RedirectResponseException.

leofeyer added a commit to contao/contao that referenced this pull request Nov 12, 2018

Respect the jump to page when closing an account (see #162)
Description
-----------

Functionality broke with contao/core-bundle#685 as `User->logout()` uses a RedirectResponseException.
Fixes #93.

Commits
-------

14f6a80 Respect jump to on close account.
da85783 Use System::getContainer() instead of Controller::getContainer()

leofeyer added a commit that referenced this pull request Nov 12, 2018

Respect the jump to page when closing an account (see #162)
Description
-----------

Functionality broke with #685 as `User->logout()` uses a RedirectResponseException.
Fixes #93.

Commits
-------

14f6a808 Respect jump to on close account.
da85783f Use System::getContainer() instead of Controller::getContainer()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment