Skip to content

New method skipAuthorizationActions - #341

Draft
alphp wants to merge 10 commits into
cakephp:3.xfrom
alphp:skipAuthorizationActions
Draft

New method skipAuthorizationActions#341
alphp wants to merge 10 commits into
cakephp:3.xfrom
alphp:skipAuthorizationActions

Conversation

@alphp

@alphp alphp commented Aug 29, 2026

Copy link
Copy Markdown

Draft, mainly to have something concrete to discuss on #339.

Comment thread composer.json
"stan-baseline": "tools/phpstan --generate-baseline",
"stan-setup": "phive install",
"rector-setup": "cp composer.json composer.backup && composer require --dev rector/rector:\"~2.3.1\" && mv composer.backup composer.json",
"rector-setup": "cp composer.json composer.backup && composer require --dev rector/rector --with-dependencies && mv composer.backup composer.json",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why was this change needed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That change was necessary for the cs-stan / Coding Standard & Static Analysis test to work and return its results.
I simply wanted to test what that test said; if necessary, I'll undo that commit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess we would need to get 3.x green again independently.

@dereuromark

Copy link
Copy Markdown
Member

Also please check your solution on:

Fatal on canResult() and authorize(). performCheck returns bool true, canResult() declares : ResultInterface:

TypeError: AuthorizationComponent::canResult(): Return value must be of type
Authorization\Policy\ResultInterface, true returned
src/Controller/Component/AuthorizationComponent.php:112

@alphp

alphp commented Aug 29, 2026

Copy link
Copy Markdown
Author

It may not be the best code, but since it's a method that must return a different type depending on the calling method, it's the only solution I could think of.

        $skipAuthorization = $this->checkAction($action, 'skipAuthorization');
        if ($skipAuthorization) {
            $this->skipAuthorization();

            return match ($method) {
                'can' => true,
                'canResult' => new Result(true),
            };
        }

@alphp

alphp commented Aug 29, 2026

Copy link
Copy Markdown
Author

I believe the intention was to unify the verification and authorization methods for controller and model actions.

This decision could lead to skipActions also affecting a model's policies, among other issues.

I wanted to write the following:

        $this->authorizeAction();
        if ($this->getService($request)->authorizationChecked()) {
            return true;
        }

But in PHPUnit, this resulted in infinite recursion: performCheck() calls authorizeAction() which calls authorize() which calls canResult which calls performCheck.

If we had two performCheck methods, since we already have two can methods, they could call two different authorizeAction methods without any unexpected recursive entries.

@ADmad

ADmad commented Aug 29, 2026

Copy link
Copy Markdown
Member

Isn't this PR effectively reimplementing what's already possible with RequestAuthorizationMiddleware?

@alphp

alphp commented Aug 29, 2026

Copy link
Copy Markdown
Author

It follows the same logic as $this->Authentication->allowUnauthenticated(['action']): it's a simple way to authorize methods that don't require explicit authorization, and it does so in the controller's beforeFilter, where the scope of its effectiveness is perfectly clear.

In a controller where many methods don't require authorization, such as the UsersController, which combines login and logout methods (and others like 2FA verification) with user management methods, it becomes very tedious to write skipAuthorization in each method or implement it in the controller's policy (you end up with many methods that only return true).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants