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

FormAuthenticator::_checkLoginUrl() fails when accessing alternative route #137

Closed
burzum opened this issue Sep 19, 2017 · 10 comments
Closed
Assignees
Labels
Milestone

Comments

@burzum
Copy link
Contributor

burzum commented Sep 19, 2017

For testing I've been accessing the direct controller/action URL http://cake3.world-architects.com/en/users/login but we have a route for that as well /en/login.

But the FormAuthenticator compares only a string URL and not if the passed array really matches the resolved controller action but instead only the first route that machtes:

\vendor\cakephp\authentication\src\Authenticator\FormAuthenticator.php (line 71)
'/en/users/login'
\vendor\cakephp\authentication\src\Authenticator\FormAuthenticator.php (line 72)
'/en/login'

I'm not sure if there is a better way than to compare the arrays to resolve this?

@markstory markstory added the bug label Sep 19, 2017
@markstory markstory added this to the 1.0.0 milestone Sep 19, 2017
@markstory
Copy link
Member

I think you would have to compare array values as well.

@burzum
Copy link
Contributor Author

burzum commented Sep 20, 2017

@markstory exactly my thought but then it is required that you run the routing middleware before authentication. But this brings up a problem if we want to do authorization against routes. authorization requires the identity.

@markstory
Copy link
Member

Couldn't the 'loginAction' be defined as a string URL if people have multiple possible routes a login page can be reached at?

@burzum
Copy link
Contributor Author

burzum commented Sep 20, 2017

We would always require a string then and turn the URI from the request also into an array and then compare them. Or am I missing something? If not I'll do the change.

@markstory
Copy link
Member

That sounds like it should work to me.

@burzum
Copy link
Contributor Author

burzum commented Sep 21, 2017

@markstory does that look OK?

    protected function _checkLoginUrl(ServerRequestInterface $request)
    {
        $loginUrl = $this->getConfig('loginUrl');

        if (!empty($loginUrl)) {
            $requestUrl = Router::parseRequest($request);

            if (is_string($loginUrl)) {
                $loginUrl = Router::parseRequest((new ServerRequest([
                    'uri' => $loginUrl
                ])));
                $this->setConfig('loginUrl', $loginUrl);
            }

            $keysToCompare = array_keys($loginUrl);

            foreach ($keysToCompare as $key) {
                if (!array_key_exists($key, $requestUrl)
                    || $requestUrl[$key] !== $loginUrl[$key]
                ) {
                    return false;
                }
            }

        }

        return true;
    }

@markstory
Copy link
Member

You could use Hash::diff or array_key_diff() to check the difference between the two URL arrays.

@burzum
Copy link
Contributor Author

burzum commented Sep 25, 2017

@markstory I've pushed the code to the branch bug/check-login-url there is an issue with the routing it seems, I'm not sure if we simply want to match against the URL it has generated in this case. Would you mind to look at the code?

@markstory markstory self-assigned this Sep 25, 2017
@markstory
Copy link
Member

Sure, I'll try to take a look in the next few days.

@markstory
Copy link
Member

Closing as #146 covers this.

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

No branches or pull requests

2 participants