CakeDC / users

Users Plugin for CakePHP
https://www.cakedc.com
Other
521 stars 297 forks source link

UsersAuthComponent::isUrlAuthorized() uses deprecated method #822

Closed ravage84 closed 4 years ago

ravage84 commented 5 years ago

CakePHP version 3.7.9 CakeDC/Users version 8.4.0

Using AuthLinkHelper::isAuthorized() or UsersAuthComponent::isUrlAuthorized() directly, leads to a Deprecated Error:


Deprecated Error: Extracting the request method from global state when parsing routes is deprecated. Instead adopt Route::parseRequest() which extracts the method from the passed request. - path\to\app\vendor\cakephp\cakephp\src\Routing\Route\DashedRoute.php, line: 65
 You can disable deprecation warnings by setting `Error.errorLevel` to `E_ALL & ~E_USER_DEPRECATED` in your config/app.php. in [path\to\app\vendor\cakephp\cakephp\src\Core\functions.php, line 311]
groovenectar commented 5 years ago

@ravage84 This warning appears to be from the CakePHP core DashedRoute class:

https://github.com/cakephp/cakephp/blob/3.7.9/src/Routing/Route/DashedRoute.php#L65

ravage84 commented 5 years ago

Correct. There should be an alternative way, which does not trigger this error.

groovenectar commented 5 years ago

I see the method here: https://github.com/CakeDC/users/blob/8.4.0/src/Controller/Component/UsersAuthComponent.php#L161

Is there any more to the stack trace?

groovenectar commented 5 years ago

Perhaps it is something in this trait?

https://github.com/CakeDC/auth/blob/master/src/Traits/IsAuthorizedTrait.php

There is an isAuthorized() method in there, which is called by the UsersAuthComponent here:

https://github.com/CakeDC/users/blob/8.4.0/src/Controller/Component/UsersAuthComponent.php#L200

groovenectar commented 5 years ago

By the way, if this is a new project, would you want to consider using the develop branch?

See the docs here where it references AuthComponent: https://github.com/CakeDC/users/blob/develop/Docs/Documentation/Configuration.md#authentication-and-authorization

You can see that in version 4 of CakePHP, AuthComponent itself is deprecated: https://book.cakephp.org/3.0/en/controllers/components/authentication.html

In CakePHP 4, the Auth will be handled using middleware, and it works in Cake 3 as well... The develop branch of this library already implements it and is just about ready for release

ravage84 commented 5 years ago

@groovenectar

Is there any more to the stack trace?

grafik

By the way, if this is a new project, would you want to consider using the develop branch?

No, it's not a new project. We can't use unstable branches.

groovenectar commented 5 years ago

That's makes sense, I was just hoping, since that's the branch I'm trying to use and help get polished and the AuthComponent method will be deprecated pretty soon. I would be in the same boat for an existing project

I see where it's calling UsersAuthComponent::isUrlAuthorized() from the view.

I don't see a usage example of it in the docs, but did find one here in the Tests: https://github.com/CakeDC/users/blob/6f77869d70eef8a9e88fa9d5ac9daa612999fede/tests/TestCase/Controller/Component/UsersAuthComponentTest.php#L130

In the test, it passes an Event, created like this:

$event = new Event('event');
$event->setData([
    'url' => '/route',
]);

Could that be related? What's getting passed in there?

Actually I wonder if running the PHPUnit tests will result in the same deprecation warning, it seems likely that it should if it's the same kind of data in your case as with the test case

ravage84 commented 5 years ago

At first, I thought it was because I had a slightly outdated CakePHP version, but after upgrading to the newest version, the issue still persisted.

Actually I wonder if running the PHPUnit tests will result in the same deprecation warning, it seems likely that it should if it's the same kind of data in your case as with the test case

I would have thought so, too. But aparently, it does not. And the reason is because the test mocks the EventManager, which prevents the actualc deprecated code being called:

https://github.com/CakeDC/users/blob/8.4.0/tests/TestCase/View/Helper/AuthLinkHelperTest.php#L134-L155

Same goes for the tests for UsersAuthComponent::isUrlAuthoirzed() with mocking AuthComponent.

https://github.com/CakeDC/users/blob/8.4.0/tests/TestCase/Controller/Component/UsersAuthComponentTest.php#L125-L514

So the code is actually well unit tested but lacks a proper integration test, interacting with the framework.

rochamarcelo commented 5 years ago

@ravage84 please provide a snippet of code showing how you are calling AuthLink helper and the specific route configuration you have.

I could not reproduce the issue.

ravage84 commented 5 years ago

@rochamarcelo the following code is what triggers the issue.

// in routes.php
$routes->connect(
    '/summary/:year/:month.csv',
    ['action' => 'downloadSummaryAsCsv'],
    ['_name' => 'downloadSummaryAsCsv']
)
->setMethods(['GET']);
// The following route config can be disabled without changing in behavior
//->setPatterns(['year' => '\d\d\d\d', 'month' => '\d{1,2}'])
//->setPass(['year', 'month']);

// in controller/view
$summaryUrl = $this->Url->build([
    'action' => 'downloadSummaryAsCsv',
    'year' => $year,
    'month' => 1,
]);

// summaryUrl = /forms/one/summary/2019/1.csv
$this->AuthLink->isAuthorized($summaryUrl);

If I remove ->setMethods(['GET']);, the deprecation warning doesn't get triggered.

The issue could be on the framework side.

rochamarcelo commented 5 years ago

Hi, I'm not sure about your url.

I could not reproduce the issue using that url. Please try removing others routes to check if other route is breaking it.

Also the 'connect' was not matching to the url correctly so I had to use '_name' instead of 'action'

$summaryUrl = $this->Url->build([
    '_name' => 'downloadSummaryAsCsv',
    'year' => $year,
    'month' => 1,
]);
rochamarcelo commented 5 years ago

Another point, the request method comes from environment variable 'REQUEST_METHOD' and it should not be empty. See ServerRequest::getMethod() at https://github.com/cakephp/cakephp/blob/master/src/Http/ServerRequest.php#L1322

ravage84 commented 5 years ago

I could not reproduce the issue using that url. Please try removing others routes to check if other route is breaking it.

Done. Yes, it is this one.

Also the 'connect' was not matching to the url correctly so I had to use '_name' instead of 'action'

If provided as array, this does not happen. And I know why now.

It checks if the URL parameter is an array, here:

https://github.com/CakeDC/users/blob/8.5.1/src/Controller/Component/UsersAuthComponent.php#L170

If it is not an array, it uses the method in question in a deprecated way:

https://github.com/CakeDC/users/blob/8.5.1/src/Controller/Component/UsersAuthComponent.php#L177

rochamarcelo commented 5 years ago

Seems that for some reason in your envirorment the 'REQUEST_METHOD' is empty, it should be 'POST' or 'GET'.

Investigating/analysing a good fix for it.

Which server (nginx, apache) and operating system are you using?

ravage84 commented 5 years ago

Windows 7 + Apache, PHP 7.1 (XAMPP)

Can't check whether it happens in other environments at the moment.

rochamarcelo commented 5 years ago

I could not reproduce the issue.

ravage84 commented 5 years ago

I will try to have a look at it when I return in two weeks.

ravage84 commented 4 years ago

Closing, as I never got around to reproduce this and never encountered again. Might be a framwork issue that as resolved in the mean time. We might never know...