Skip to content

PRS-2 reformatting of code #58

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

PRS-2 reformatting of code #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51f5d58
PRS-2 reformatting of code
abolabo Dec 25, 2018
138cfc8
composer.json chng
abolabo Dec 25, 2018
c66fd5c
composer.json chng
abolabo Dec 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 36 additions & 32 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,39 @@
{
"name": "craftcamp/php-abac",
"description": "Library used to implement Attribute-Based Access Control in a PHP application",
"type": "library",
"keywords": ["access-control", "attributes", "security"],
"license": "MIT",
"minimum-stability": "stable",
"authors": [
{
"name": "Axel Venet",
"email": "[email protected]",
"role": "Developer"
}
],
"require": {
"php": ">=7.0",
"psr/cache": "~1.0",
"symfony/config": "~3.0|^4.0",
"symfony/yaml" : "~3.0|^4.0",
"friendsofphp/php-cs-fixer": "^2.12"
},
"support": {
"email": "[email protected]"
},
"autoload" : {
"psr-4": {
"PhpAbac\\": "src/",
"PhpAbac\\Example\\": "example/",
"PhpAbac\\Test\\": "tests/"
}
},
"require-dev": {
"phpunit/phpunit": "^6.5"
"name": "craftcamp/php-abac",
"description": "Library used to implement Attribute-Based Access Control in a PHP application",
"type": "library",
"keywords": [
"access-control",
"attributes",
"security"
],
"license": "MIT",
"minimum-stability": "stable",
"authors": [
{
"name": "Axel Venet",
"email": "[email protected]",
"role": "Developer"
}
],
"require": {
"php": ">=7.0",
"psr/cache": "~1.0",
"symfony/config": "~3.0|^4.0",
"symfony/yaml": "~3.0|^4.0",
"friendsofphp/php-cs-fixer": "^2.12"
},
"support": {
"email": "[email protected]"
},
"autoload": {
"psr-4": {
"PhpAbac\\": "src/",
"PhpAbac\\Example\\": "example/",
"PhpAbac\\Test\\": "tests/"
}
},
"require-dev": {
"phpunit/phpunit": "^6.5"
}
}
86 changes: 69 additions & 17 deletions src/Abac.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,60 @@

use PhpAbac\Manager\{
AttributeManager,
AttributeManagerInterface,
CacheManager,
CacheManagerInterface,
ComparisonManager,
PolicyRuleManager
ComparisonManagerInterface,
PolicyRuleManager,
PolicyRuleManagerInterface
};
use PhpAbac\Model\PolicyRule;
use PhpAbac\Model\PolicyRuleAttribute;

final class Abac
{
/** @var PolicyRuleManager **/
/**
* @var PolicyRuleManager|PolicyRuleManager
*/
private $policyRuleManager;
/** @var AttributeManager **/
/**
* @var AttributeManager|AttributeManagerInterface
Copy link
Collaborator

@Kern046 Kern046 Dec 26, 2018

Choose a reason for hiding this comment

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

It is not useful to set the Manager in PHPdoc if the interface is present. You can remove it :)

Copy link
Author

@abolabo abolabo Dec 27, 2018

Choose a reason for hiding this comment

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

hm.. i think you are not right. Phpdoc comments used by IDE such as PhpStorm to show relations between different code. All logic of code-inspectors based on phpdocumentor comments. So if you will run inspector it will show a lot of errors "unknown classes" etc.
Anyway relations of code is important thing if you decided to rename (refactor) some of class.
(Or in case when you call some class not directly, dynamically)

Copy link
Collaborator

@Kern046 Kern046 Dec 27, 2018

Choose a reason for hiding this comment

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

Sorry I wasn't precise enough. I just said that you could do:

/** @var AttributeManagerInterface **/ and not mention `AttributeManager`` since the purpose to set interfaces instead of classes directly is to remove the dependency to a precise class.

I have no doubts on the utility of PHPdoc comments to declare properties type. It's just for methods where I suggest that PHP7 type-hinting is enough for modern IDEs

Copy link
Author

@abolabo abolabo Dec 27, 2018

Choose a reason for hiding this comment

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

oh, yes. But in this case IDE will not hint methods of some sibling class of interface.
I mean quick jump to class method from call by ide shortcut ( ctrl+click ). If i describe parameter only as interface class ide jumps into interface class, not child.
Anyway it's your code, your rules :-)

Copy link
Contributor

@antonkomarev antonkomarev Dec 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There is no need to specify all siblings in DocBlock. Only interfaces is fine.
After redirecting in IDE to interface you can choose on what sibling you want to redirect further.

Copy link
Contributor

@antonkomarev antonkomarev Dec 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It's good practice to have FQCN (Fully Qualified Class Names) in DocBlocks.

Copy link
Collaborator

@Kern046 Kern046 Dec 27, 2018

Choose a reason for hiding this comment

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

@abolabo The matter in open-source is not whose code it is, but what is the best way to code this library, and I'm totally open to changes even the ones I wouldn't do myself if it is the best thing according to conventions etc...

Your PR is good, it is totally right to apply SOLID principles here. You did transform the class type-hint into interfaces to remove hard dependencies between classes and it is essential to allow overriding of the library components. To be coherent with this change, the PHPdoc must also be updated, but must not keep the old hard dependency to the classes such as AttributeManager.

In the meantime, you can remove the classes use statements which would become useless with the replacement by interfaces.

Copy link
Collaborator

@Kern046 Kern046 Dec 27, 2018

Choose a reason for hiding this comment

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

I understand your concern about jumps in IDE but if someone overrides AttributeManager and set it as Abac dependency, it would be wrong that the IDE jumps to the native AttributeManager instead of the extended one. Jumping to the interface seems to be more coherent

*/
private $attributeManager;
/** @var CacheManager **/
/**
* @var CacheManager| CacheManagerInterface
*/
private $cacheManager;
/** @var ComparisonManager **/
/**
* @var ComparisonManager | ComparisonManagerInterface
*/
private $comparisonManager;
/** @var array **/
/**
* @var array
*/
private $errors;

public function __construct(PolicyRuleManager $policyRuleManager, AttributeManager $attributeManager, ComparisonManager $comparisonManager, CacheManager $cacheManager)
{

/**
* Abac constructor.
Copy link
Collaborator

@Kern046 Kern046 Dec 26, 2018

Choose a reason for hiding this comment

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

Is it really useful to say that this is the class constructor ?

*
* @param PolicyRuleManagerInterface $policyRuleManager
* @param AttributeManagerInterface $attributeManager
* @param ComparisonManagerInterface $comparisonManager
* @param CacheManagerInterface $cacheManager
*/
public function __construct(
PolicyRuleManagerInterface $policyRuleManager,
AttributeManagerInterface $attributeManager,
ComparisonManagerInterface $comparisonManager,
CacheManagerInterface $cacheManager
){
$this->attributeManager = $attributeManager;
$this->policyRuleManager = $policyRuleManager;
$this->cacheManager = $cacheManager;
$this->comparisonManager = $comparisonManager;
}

/**
* Return true if both user and object respects all the rules conditions
* If the objectId is null, policy rules about its attributes will be ignored
Expand All @@ -45,6 +72,13 @@ public function __construct(PolicyRuleManager $policyRuleManager, AttributeManag
*
* Available cache drivers are :
* * memory
*
* @param string $ruleName
* @param object $user
* @param null|object $resource
* @param array $options
*
* @return bool
*/
public function enforce(string $ruleName, $user, $resource = null, array $options = []): bool
{
Expand All @@ -55,8 +89,14 @@ public function enforce(string $ruleName, $user, $resource = null, array $option
$this->comparisonManager->setDynamicAttributes($options[ 'dynamic_attributes' ]);
}
// Retrieve cache value for the current rule and values if cache item is valid
if (($cacheResult = isset($options[ 'cache_result' ]) && $options[ 'cache_result' ] === true) === true) {
$cacheItem = $this->cacheManager->getItem("$ruleName-{$user->getId()}-" . (($resource !== null) ? $resource->getId() : ''), (isset($options[ 'cache_driver' ])) ? $options[ 'cache_driver' ] : null, (isset($options[ 'cache_ttl' ])) ? $options[ 'cache_ttl' ] : null);
$cacheResult = (isset($options[ 'cache_result' ]) && $options[ 'cache_result' ] === true);

if ($cacheResult === true) {
$cacheItem = $this->cacheManager->getItem(
"$ruleName-{$user->getId()}-" . (($resource !== null) ? $resource->getId() : ''),
(isset($options[ 'cache_driver' ])) ? $options[ 'cache_driver' ] : null,
(isset($options[ 'cache_ttl' ])) ? $options[ 'cache_ttl' ] : null
);
// We check if the cache value s valid before returning it
if (($cacheValue = $cacheItem->get()) !== null) {
return $cacheValue;
Expand All @@ -66,12 +106,17 @@ public function enforce(string $ruleName, $user, $resource = null, array $option

foreach ($policyRules as $policyRule) {
// For each policy rule attribute, we retrieve the attribute value and proceed configured extra data
/**
* @var PolicyRule $policyRule
*/
foreach ($policyRule->getPolicyRuleAttributes() as $pra) {
/** @var PolicyRuleAttribute $pra */
$attribute = $pra->getAttribute();

$getter_params = $this->prepareGetterParams($pra->getGetterParams(), $user, $resource);
$attribute->setValue($this->attributeManager->retrieveAttribute($attribute, $user, $resource, $getter_params));
$attribute->setValue(
$this->attributeManager->retrieveAttribute($attribute, $user, $resource, $getter_params)
);
if (count($pra->getExtraData()) > 0) {
$this->processExtraData($pra, $user, $resource);
}
Expand All @@ -97,7 +142,8 @@ public function getErrors(): array
}

/**
* Function to prepare Getter Params when getter require parameters ( this parameters must be specified in configuration file)
* Function to prepare Getter Params when getter require parameters
* ( this parameters must be specified in configuration file)
*
* @param $getter_params
* @param $user
Expand All @@ -116,7 +162,12 @@ private function prepareGetterParams($getter_params, $user, $resource)
if ('@' !== $param[ 'param_name' ][ 0 ]) {
$values[$getter_name][] = $param[ 'param_value' ];
} else {
$values[$getter_name][] = $this->attributeManager->retrieveAttribute($this->attributeManager->getAttribute($param[ 'param_value' ]), $user, $resource);
$values[$getter_name][] = $this->attributeManager
->retrieveAttribute(
$this->attributeManager->getAttribute($param[ 'param_value' ]),
$user,
$resource
);
}
}
}
Expand All @@ -134,8 +185,8 @@ private function processExtraData(PolicyRuleAttribute $pra, $user, $resource)
// The "with" extra data is an array of attributes, which are objects
// Once we process it as policy rule attributes, we set it as the main policy rule attribute value
$subPolicyRuleAttributes = [];

foreach ($this->policyRuleManager->processRuleAttributes($data, $user, $resource) as $subPolicyRuleAttribute) {
$subs = $this->policyRuleManager->processRuleAttributes($data, $user, $resource);
foreach ($subs as $subPolicyRuleAttribute) {
$subPolicyRuleAttributes[] = $subPolicyRuleAttribute;
}
$pra->setValue($subPolicyRuleAttributes);
Expand All @@ -147,4 +198,5 @@ private function processExtraData(PolicyRuleAttribute $pra, $user, $resource)
}
}
}

}
102 changes: 83 additions & 19 deletions src/AbacFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

use PhpAbac\Configuration\ConfigurationInterface;
use PhpAbac\Configuration\Configuration;

use PhpAbac\Manager\PolicyRuleManager;
use PhpAbac\Manager\PolicyRuleManagerInterface;
use PhpAbac\Manager\AttributeManager;
Expand All @@ -14,53 +13,118 @@
use PhpAbac\Manager\ComparisonManager;
use PhpAbac\Manager\ComparisonManagerInterface;


/**
* Class AbacFactory
*
* @package PhpAbac
*/
final class AbacFactory
{
/** @var ConfigurationInterface **/
/**
* @var ConfigurationInterface
*/
protected static $configuration;
/** @var PolicyRuleManagerInterface **/
/**
* @var PolicyRuleManagerInterface *
*/
protected static $policyRuleManager;
/** @var AttributeManagerInterface **/
/**
* @var AttributeManagerInterface *
*/
protected static $attributeManager;
/** @var CacheManagerInterface **/
/**
* @var CacheManagerInterface *
*/
protected static $cacheManager;
/** @var ComparisonManagerInterface **/
/**
* @var ComparisonManagerInterface *
*/
protected static $comparisonManager;


/**
* @param ConfigurationInterface $configuration
*/
public static function setConfiguration(ConfigurationInterface $configuration)
{
self::$configuration = $configuration;
}


/**
* @param PolicyRuleManagerInterface $policyRuleManager
*/
public static function setPolicyRuleManager(PolicyRuleManagerInterface $policyRuleManager)
{
self::$policyRuleManager = $policyRuleManager;
}


/**
* @param AttributeManagerInterface $attributeManager
*/
public static function setAttributeManager(AttributeManagerInterface $attributeManager)
{
self::$attributeManager = $attributeManager;
}


/**
* @param CacheManagerInterface $cacheManager
*/
public static function setCacheManager(CacheManagerInterface $cacheManager)
{
self::$cacheManager = $cacheManager;
}


/**
* @param ComparisonManagerInterface $comparisonManager
*/
public static function setComparisonManager(ComparisonManagerInterface $comparisonManager)
{
self::$comparisonManager = $comparisonManager;
}

public static function getAbac(array $configurationFiles, string $configDir = null, array $attributeOptions = [], array $cacheOptions = [])

/**
* @param array $configurationFiles
* @param string|null $configDir
* @param array $attributeOptions
* @param array $cacheOptions
*
* @return Abac
*/
public static function getAbac(
array $configurationFiles,
string $configDir = null,
array $attributeOptions = [],
array $cacheOptions = [])
{
$configuration = (self::$configuration !== null) ? self::$configuration : new Configuration($configurationFiles, $configDir);
$attributeManager = (self::$attributeManager !== null) ? self::$attributeManager : new AttributeManager($configuration, $attributeOptions);
$policyRuleManager = (self::$policyRuleManager !== null) ? self::$policyRuleManager : new PolicyRuleManager($configuration, $attributeManager);
$comparisonManager = (self::$comparisonManager !== null) ? self::$comparisonManager : new ComparisonManager($attributeManager);
$cacheManager = (self::$cacheManager !== null) ? self::$cacheManager : new CacheManager($cacheOptions);
$configuration =
(self::$configuration !== null)
? self::$configuration
: new Configuration($configurationFiles, $configDir);

$attributeManager =
(self::$attributeManager !== null)
? self::$attributeManager
: new AttributeManager($configuration, $attributeOptions);

$policyRuleManager =
(self::$policyRuleManager !== null)
? self::$policyRuleManager
: new PolicyRuleManager($configuration, $attributeManager);

$comparisonManager =
(self::$comparisonManager !== null)
? self::$comparisonManager
: new ComparisonManager($attributeManager);

$cacheManager =
(self::$cacheManager !== null)
? self::$cacheManager
: new CacheManager($cacheOptions);

return new Abac($policyRuleManager, $attributeManager, $comparisonManager, $cacheManager);
}

public static function getRules()
{
return self::$configuration != null ? self::$configuration->getRules() : [];
}
}
Loading