-
Notifications
You must be signed in to change notification settings - Fork 4
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: Add RedirectMiddleware and make Flow/Neos 7 compatible #7
Conversation
albe
commented
Apr 14, 2021
- Add middleware
- Fix/improve communication from Provider to Middleware for redirect
- Remove old component
- Update Settings
{ | ||
$response = $next->handle($request); | ||
// TODO: Find a better solution to communicate from TokenProvider to this middleware | ||
$redirectTarget = $response->getHeaderLine(static::class . '.redirect'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See neos/flow-development-collection#2019 (comment)
Though we do not even have access to the ActionResponse
in the TokenProvider, so this might become a bit harder to achieve than anticipated. If anyone has a good idea, just shoot.
I'm still thinking if it wouldn't make more sense to use the entryPoint
configuration for the login redirect and use a middleware that checks for the setup redirect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this be solved with exceptions, as is the case here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@albe @johannessteu
I think it could work like that:
master...gerdemann:Neos7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using exceptions for control flow is a bit quirky because of the overhead of throwing/catching exceptions (i.e. they have to create a full stack trace).
But in this case I also think that it's the nicer solution – and one top-level exception shouldn't do any harm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@albe Do you want to adapt your PR or should I create a new one? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gerdemann btw I think your solution could be simplified to
public function process(ServerRequestInterface $request, RequestHandlerInterface $next): ResponseInterface
{
try {
$response = $next->handle($request);
} catch (\Exception $exception) {
if ($exception instanceof SecondFactorLoginException || $exception->getPrevious() instanceof SecondFactorLoginException) {
return $this->redirectToLogin($request);
} elseif ($exception instanceof SecondFactorSetupException || $exception->getPrevious() instanceof SecondFactorSetupException) {
return $this->redirectToSetup($request);
} else {
throw $exception;
}
}
return $response;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I have now adjusted this and created a PR. It doesn't matter to me whether this PR is adjusted or mine is merged. 😃
#8
Closing in favor of #8 |