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

TASK: Refactor to fusion #26

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
59 changes: 28 additions & 31 deletions Classes/Controller/AuthenticationController.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,18 @@
use Neos\Flow\I18n\Translator;
use Neos\Flow\Mvc\ActionRequest;
use Neos\Flow\Mvc\Exception\UnsupportedRequestTypeException;
use Neos\Flow\Mvc\FlashMessage\FlashMessageService;
use Neos\Flow\Mvc\View\ViewInterface;
use Neos\Flow\Security\Authentication\Controller\AbstractAuthenticationController;
use Neos\Flow\Security\Exception\AuthenticationRequiredException;
use Neos\Fusion\View\FusionView;

/**
* Controller for displaying a login/logout form and authenticating/logging out "frontend users"
*/
class AuthenticationController extends AbstractAuthenticationController
{

/**
* @var Translator
* @Flow\Inject
Expand All @@ -45,12 +49,10 @@ class AuthenticationController extends AbstractAuthenticationController
protected $translationSourceName;

/**
* @return void
* @Flow\Inject
* @var FlashMessageService
*/
public function indexAction(): void
{
$this->view->assign('account', $this->securityContext->getAccount());
}
protected $flashMessageService;

/**
* return void
Expand All @@ -60,12 +62,7 @@ public function logoutAction()
parent::logoutAction();

$uri = $this->request->getInternalArgument('__redirectAfterLogoutUri');

if (empty($uri)) {
$this->redirect('index');
} else {
$this->redirectToUri($uri);
}
$this->redirectToUri($uri);
}

/**
Expand All @@ -76,36 +73,36 @@ public function logoutAction()
protected function onAuthenticationSuccess(ActionRequest $originalRequest = null)
{
$uri = $this->request->getInternalArgument('__redirectAfterLoginUri');

if (empty($uri)) {
$this->redirect('index');
} else {
$this->redirectToUri($uri);
}
$this->redirectToUri($uri);
}

/**
* Create translated FlashMessage and add it to flashMessageContainer
* Create add a validation error and send the request back to the referrer
*
* @param AuthenticationRequiredException $exception
* @return void
*/
protected function onAuthenticationFailure(AuthenticationRequiredException $exception = null)
{
$title = $this->getTranslationById('authentication.failure.title');
$message = $this->getTranslationById('authentication.failure.message');
$this->addFlashMessage($message, $title, Error\Message::SEVERITY_ERROR, [], $exception === null ? 1496914553 : $exception->getCode());
}
$referringRequest = $this->request->getReferringRequest();
if ($referringRequest === null) {
return;
}

/**
* Get translation by label id for configured source name and package key
*
* @param string $labelId Key to use for finding translation
* @return string Translated message or NULL on failure
*/
protected function getTranslationById($labelId): string
{
return $this->translator->translateById($labelId, [], null, null, $this->translationSourceName, $this->translationPackageKey);
$validationResults = new Error\Result();
$validationResults->addError(new Error\Error('authenticationFailure'));

$packageKey = $referringRequest->getControllerPackageKey();
$subpackageKey = $referringRequest->getControllerSubpackageKey();
if ($subpackageKey !== null) {
$packageKey .= '\\' . $subpackageKey;
}

$argumentsForNextController = $referringRequest->getArguments();
$argumentsForNextController['__submittedArguments'] = [];
$argumentsForNextController['__submittedArgumentValidationResults'] = $validationResults;

$this->forward($referringRequest->getControllerActionName(), $referringRequest->getControllerName(), $packageKey , $argumentsForNextController);
}

/**
Expand Down
21 changes: 21 additions & 0 deletions Configuration/Routes.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@


-
name: 'Frontend Authenticate'
uriPattern: 'frontend/login(.{@format})'
defaults:
'@package': 'Flowpack.Neos.FrontendLogin'
'@controller': 'Authentication'
'@action': 'authenticate'
'@format': 'html'
httpMethods: ['POST']

-
name: 'Frontend Logout'
uriPattern: 'frontend/logout(.{@format})'
defaults:
'@package': 'Flowpack.Neos.FrontendLogin'
'@controller': 'Authentication'
'@action': 'logout'
'@format': 'html'
httpMethods: ['POST']
4 changes: 4 additions & 0 deletions Configuration/Settings.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ Neos:
pattern: Flowpack\Neos\FrontendLogin\Security\NeosRequestPattern
patternOptions:
matchFrontend: true
mvc:
routes:
Flowpack.Neos.FrontendLogin:
position: 'before Neos.Neos'
Neos:
userInterface:
translation:
Expand Down
137 changes: 123 additions & 14 deletions Resources/Private/Fusion/NodeTypes/LoginForm.fusion
Original file line number Diff line number Diff line change
@@ -1,16 +1,125 @@
##
# "LoginForm" element, extending "Plugin"
# "LoginForm" element, extending "ContentComponent"
#
prototype(Flowpack.Neos.FrontendLogin:LoginForm) < prototype(Neos.Neos:Plugin) {
package = 'Flowpack.Neos.FrontendLogin'
controller = 'Authentication'
action = 'index'

redirectAfterLoginUri = Neos.Neos:NodeUri {
node = ${q(node).property('redirectAfterLogin')}
}

redirectAfterLogoutUri = Neos.Neos:NodeUri {
node = ${q(node).property('redirectAfterLogout')}
}
}
prototype(Flowpack.Neos.FrontendLogin:LoginForm) < prototype(Neos.Neos:ContentComponent) {

loginPrototype = 'Flowpack.Neos.FrontendLogin:LoginForm.Login'
logoutPrototype = 'Flowpack.Neos.FrontendLogin:LoginForm.Logout'

renderer = Neos.Fusion:Case {
loggedIn {
condition = ${Security.hasRole('Flowpack.Neos.FrontendLogin:User')}
type = ${props.logoutPrototype}
element {
accountIdentifier = ${Security.getAccount().accountIdentifier}
redirectAfterLogoutUri = Neos.Neos:NodeUri {
node = ${q(node).property('redirectAfterLogout')}
}
}
}

default {
condition = true
type = ${props.loginPrototype}
element {
redirectAfterLoginUri = Neos.Neos:NodeUri {
node = ${q(node).property('redirectAfterLogin')}
Copy link
Member

Choose a reason for hiding this comment

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

This will not work when the target page needs the front-end user privilege.
In this case a url to the login page is created instead.

So I think we have to access the node in the authentication controller somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, maybe getting rid of the plugin has some side effects :-/ not sure how to solve this safe and sane.

Copy link
Member Author

Choose a reason for hiding this comment

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

A way around would be to drop the redirectAfter-feature and just always send the user back to the referer wich uses arguments that are signed with an hmac.

However finding a way to transport this safely to the controller without relying on plugins would be a really nice example for such cases.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder a bit how it worked before. Did that work before?

Other ways:

  • Providing a linkeditor for both uris would of course also make it possible to give a relative url. Not the best solution but maybe would anyway make sense for other usecases.
  • Give the login form node identifier to the controller and let it build the uri from its properties when it has the required privileges. Would require us to duplicate some code from the NodeUri builder I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

The plugin evaluates the path redirectAfterLoginUri after the login and not immediately. Maybe we could create a lazyEvaluationProxy fusionObject as wrapper for the NodeUri :-/

BTW: I just found out that the FusionTemplate does similar things for all untyped properties.

Copy link
Member Author

@mficzel mficzel Dec 9, 2019

Choose a reason for hiding this comment

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

Core of the problem is that props are usually evaluated immediately and not once they are accessed. Changing that is not that simple as you can see here neos/neos-development-collection#2738

Copy link
Member

Choose a reason for hiding this comment

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

So which way should we go then. Back to the plugin for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Either use your original pr or find a solution here which will take time and a sprint would be a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

But then I would prefer the plugin approach for now. Would love to have this and not wait 5-6 months.
With some more experience from this we also have a better idea until the sprint to remove the plugin part.

Do you want to create another PR with my commit + your cleanup adjustments?

}
}
}
}

@cache {
mode = 'dynamic'
entryIdentifier {
node = ${node}
}
entryDiscriminator = ${(Security.hasRole('Flowpack.Neos.FrontendLogin:User') || request.internalArguments.__submittedArgumentValidationResults.errors) ? false : 'no-fe-user'}
context {
1 = 'node'
2 = 'documentNode'
}
entryTags {
1 = ${Neos.Caching.nodeTag(node)}
}
}
}

prototype(Flowpack.Neos.FrontendLogin:LoginForm.Login) < prototype(Neos.Fusion:Component) {

redirectAfterLoginUri = null

settings = ${Configuration.setting('Flowpack.Neos.FrontendLogin')}

renderer = afx`
<Neos.Fusion.Form:Form
form.target.package="Flowpack.Neos.FrontendLogin"
form.target.controller="Authentication"
form.target.action="authenticate"
attributes.role="form"
>
<Neos.Fusion.Form:Hidden field.name="__redirectAfterLoginUri" field.value={props.redirectAfterLoginUri} />

<div @if.hasErrors={form.hasErrors()}>
<p><strong>
{I18n.id('authentication.failure.title').package(props.settings.translation.packageKey).source(props.settings.translation.sourceName).translate()}
</strong></p>
<p>
{I18n.id('authentication.failure.message').package(props.settings.translation.packageKey).source(props.settings.translation.sourceName).translate()}
</p>
</div>

<div>
<label for="flowpack-neos-frontendlogin-username" class="col-sm-2 control-label">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<label for="flowpack-neos-frontendlogin-username" class="col-sm-2 control-label">
<label for="flowpack-neos-frontendlogin-username">

{I18n.id('authentication.form.username').package(props.settings.translation.packageKey).source(props.settings.translation.sourceName).translate()}:
</label>
<Neos.Fusion.Form:Input
field.name="__authentication[Neos][Flow][Security][Authentication][Token][UsernamePassword][username]"
attributes.id="flowpack-neos-frontendlogin-username"
/>
</div>
<div>
<label for="flowpack-neos-frontendlogin-password" class="col-sm-2 control-label">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<label for="flowpack-neos-frontendlogin-password" class="col-sm-2 control-label">
<label for="flowpack-neos-frontendlogin-password">

{I18n.id('authentication.form.password').package(props.settings.translation.packageKey).source(props.settings.translation.sourceName).translate()}:
</label>
<Neos.Fusion.Form:Password
field.name="__authentication[Neos][Flow][Security][Authentication][Token][UsernamePassword][password]"
attributes.id="flowpack-neos-frontendlogin-password"
/>
</div>
<div>
<Neos.Fusion.Form:Button>
{I18n.id('authentication.form.submit').package(props.settings.translation.packageKey).source(props.settings.translation.sourceName).translate()}
</Neos.Fusion.Form:Button>
</div>
</Neos.Fusion.Form:Form>
`
}

prototype(Flowpack.Neos.FrontendLogin:LoginForm.Logout) < prototype(Neos.Fusion:Component) {

redirectAfterLogoutUri = null
accountIdentifier = null

settings = ${Configuration.setting('Flowpack.Neos.FrontendLogin')}

renderer = afx`
<p>
{I18n.id('authentication.form.loggedIn').value('You are logged in as').package(props.settings.translation.packageKey).source(props.settings.translation.sourceName).translate()}&nbsp;
<strong>{props.accountIdentifier}</strong>
</p>
<Neos.Fusion.Form:Form
form.target.package="Flowpack.Neos.FrontendLogin"
form.target.controller="Authentication"
form.target.action="logout"
attributes.class="form-horizontal clearfix"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
attributes.class="form-horizontal clearfix"

attributes.role="form"
>
<Neos.Fusion.Form:Hidden field.name="__redirectAfterLogoutUri" field.value={props.redirectAfterLogoutUri} />

<Neos.Fusion.Form:Button>
{I18n.id('authentication.form.logout').value('Logout').package(props.settings.translation.packageKey).source(props.settings.translation.sourceName).translate()}
</Neos.Fusion.Form:Button>
</Neos.Fusion.Form:Form>
`
}
39 changes: 0 additions & 39 deletions Resources/Private/Templates/Authentication/Index.html

This file was deleted.

6 changes: 4 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
"description": "Neos plugin demonstrating a simple frontend login",
"license": "MIT",
"require": {
"neos/neos": "^5.0 || dev-master"
"neos/neos": "^5.0 || dev-master",
"neos/fusion-form": "^1.0 || dev-master",
"neos/fusion-afx": "^1.4 || dev-master"
},
"autoload": {
"psr-4": {
Expand All @@ -13,7 +15,7 @@
},
"extra": {
"branch-alias": {
"dev-master": "4.0.x-dev"
"dev-master": "5.0.x-dev"
},
"applied-flow-migrations": [
"TYPO3.FLOW3-201201261636",
Expand Down