Bug #47995

The method hasRole() in TYPO3/Flow/Security/Account is currently broken

Added by Dominique Feyer over 6 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
Should have
Category:
-
Target version:
-
Start date:
2013-05-06
Due date:
% Done:

0%

PHP Version:
Has patch:
Yes
Complexity:

Description

Currently this code always return FALSE:

$account->hasRole(new \TYPO3\Flow\Security\Policy\Role('Your.Package:Role')

The hasRole method use the contains method from Doctrine ArrayCollection. Has the two object are different (even is they have the same identifier), the method return FALSE.

I propose to change this method by the same feature found in the SecurityContext hasRole method (who compore the identifer, and the full object).

History

#1 Updated by Gerrit Code Review over 6 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/20555

#2 Updated by Gerrit Code Review over 6 years ago

Patch set 2 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/20555

#3 Updated by Gerrit Code Review over 6 years ago

Patch set 3 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/20555

#4 Updated by Bastian Waidelich over 6 years ago

  • Status changed from Under Review to Needs Feedback

The actual "issue" is that roles are entities since #28862
Account::hasRole() won't be the only place where this breaks compatibility.
For instance:

$account->addRole(new \TYPO3\Flow\Security\Policy\Role('Your.Package:Role');

won't work either - as well as Account::setRoles(), Account::removeRole() and so on.
The way to work around this is not to create new instances but fetch existing roles from the PolicyService:

$role = $policyService->getRole('Your.Package:Role');
$account->hasRole($role);

IMO we need to document this before the release an add visible pointers to the documentation in the release notes

Note: with ValueObjects this might be less breaking but they're currently not properly supported AFAIR

#5 Updated by Christian Müller over 6 years ago

We could try to use VO here. That should solve the mentioned problem. At least for doctrine our implementation should do what is needed.

#6 Updated by Karsten Dambekalns over 6 years ago

One thing we could discuss is what methods should (alternatively) be able to take a string (role identifier). But then again, checking for roles is not really a clever thing in most cases.

Why? Because it ignores the fact that roles might get reconfigured permissions. So if you want to check for access to something, you should rather check if permission to access things is granted, not if a certain role is assigned!

#7 Updated by Bastian Waidelich over 6 years ago

Karsten Dambekalns wrote:

if you want to check for access to something, you should rather check if permission to access things is granted, not if a certain role is assigned!

I agree!
Unfortunately the API for this is not very usable yet (at least some convenience method would do good). Currently I have to add this snippet in my base controllers:

/**
 * Check if we currently have access to the given resource
 *
 * @param string $resource The resource to check
 * @return boolean TRUE if we currently have access to the given resource
 */
protected function hasAccessToResource($resource) {
    try {
        $this->accessDecisionManager->decideOnResource($resource);
    } catch (AccessDeniedException $e) {
        return FALSE;
    }
    return TRUE;
}

#8 Updated by Karsten Dambekalns over 6 years ago

Bastian Waidelich wrote:

Unfortunately the API for this is not very usable yet (at least some convenience method would do good). Currently I have to add this snippet in my base controllers:
[...]

Full Ack. What about adding that to some clever place?

#9 Updated by Dominique Feyer over 6 years ago

Thanks for all those information, I update my application based on it, and everything work fine.

I need to spend more time on the decisionMaker, but currently my role configuration are really simple so I can rely on simple hasRole logic.

All the information in this issue will have a better visibility in the official documentation I think. The change around Role can be frustrating without that.

#10 Updated by Bastian Waidelich over 6 years ago

  • Status changed from Needs Feedback to Closed

Thanks for the feedback Dominique,

We'll definitely have to add a comprehensive "breaking changes" section to documentation & release notes with code examples and such.

Also available in: Atom PDF