Redirect refactor #135

Closed
wants to merge 2 commits into
from

Projects

None yet

6 participants

@Danielss89
ZF-Commons member

Edited redirect to use sessions instead of GET.

@shipleyr

I'm struggling to follow this change a little. Doesn't this completely break the redirect parameter? If someone constructs a link "/user/login?redirect=/" how is this handled by this change? At present it picks up the parameter and then when someone logs in they get redirected to or if this is not set to the login redirect route set in the config.

@Danielss89
ZF-Commons member

@shipleyr its not handled. I've removed that :) If the developer wants to redirect, he should use a session.

@shipleyr

Can I ask what the motivation was behind removing the redirect parameter? If it makes sense then I may well consider that I need to make a change in my application. I would rather not see the functionality removed though!

@Danielss89
ZF-Commons member

I.e. it allows for injecting of redirection. A "hacker" could send a link which would redirect to another site when the user logs in. This is not possible if the redirection is done with sessions.

@shipleyr

Is that an issue. The redirect is controlled by the redirect helper so the page will always be within the same application? (I'm not 100% on this... is it possible to redirect to another site using a redirect helper?). It's getting to the end of my knowledge on this area but I'd be slightly reluctant to loose the redirect functionality via a get parameter.

My use case was something like having a link on every page (for non-logged in users) to allow people to login and then redirect back to the page the user was on prior to being redirected. Using a session variable would mean having to set and change a session parameter on every request. I'd appreciate some other thoughts on this. @EvanDotPro any thoughts?

@jessertaylor

nice, shame these are not being committed.

@akrabat

I think that redirecting via sessions should to be optional. I follow @shipleyr's argument that having to keep setting the session var for every page load would get tedious.

@Freeaqingme

I just commented on #241, which states a similar issue. If multiple domains (and applications) are involved (e.g. SSO), working with sessions for redirect url's is not an option. As mentioned in #241 I'd propose to proceed with GET parameters, but use HMAC's instead for reasons of security.

@DaanBiesterbos

Keeping track of browse history may seem more difficult then it is. I had the same problem not too long ago. I solved it by writing a small module and attached it to the route event. Here are two snippets.

/* Module::onBootstrap */
$eventManager->attach('route', function(MvcEvent $e){
$routeMatch = $e->getRouteMatch();
if(!empty($routeMatch)){

            // Create pageview
            $pageView = new PageView();
            $pageView->setRouteName( $routeMatch->getMatchedRouteName() );
            $pageView->setParams(  $routeMatch->getParams() );
            $pageView->setQuery( $e->getRequest()->getQuery()->toArray() );
            $pageView->setRequestTime( $_SERVER['REQUEST_TIME'] );

            // Register pageview
            $historyService = $e->getApplication()->getServiceManager()->get('ltUserHistory\UserHistory');
            $historyService->registerPageView($pageView);

        }

}, 0);

A helper can be used to get the current or previous pageviews...
I used the following:

public function __invoke($steps){
// 0 is current page.
$steps = intval($steps);
$pageView = $this->getServiceManager()
->getServiceLocator()
->get('ltUserHistory\UserHistory')
->getPageView($steps);

    if(!empty($pageView)){          
        $routeName = $pageView->getRouteName();
        $params = $pageView->getParams();
        $query = $pageView->getQuery();
        $query = http_build_query($query);
        $url = $this->getView()->url($routeName, $params); 
        $url .= (!empty($query)) ? '?'.$query : '';
        return $url;
    }

}

@Danielss89 Danielss89 closed this Mar 24, 2013
@spiffyjr spiffyjr referenced this pull request Jun 21, 2013
Closed

redirect to other domain #241

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment