Bonjour,
Dans le cadre de tests de montée en version GLPI 0.83 vers 0.84, j'ai
rencontré une régression dans l'application des règles "Automatic user
assignment" sur l'affectation des entités à partir d'une expression régulière
vérifiant le DN de l'utilisateur.
Le résultat de cette régression est que les entités affectées par expression
régulière se retrouvent tronquées (ex: Entité 11 => Entité 1, Entité 25 =>
Entité 2).
Je pense avoir réglé ce problème avec le patch ci-joint [0001-fix-and-clean-
ruleright-code.patch] applicable sur la
révision 21240 et la 0.84-RC3.
En parcourant le code relatif à ces règles (cf. inc/user.class.php,
inc/ruleright.class.php et inc/entity.class.php), j'ai aussi tenté d'améliorer
certains points qui rendait la compréhension de ces règles compliquées :
- Le résultat de la méthode RuleRight::executeActions() renvoie actuellement 3
tableaux nommés différemment en fonction de la présence d'entités et/ou de
profils, qui sont ensuite traités différemment dans la fonction
User::applyRightRules().
J'ai simplifié le retour de la fonction RuleRight::executeActions() en ne
renvoyant qu'un seul tableau et en déléguant les diverses conditions
d'affectation uniquement à la méthode User::applyRightRules().
- La boucle foreach sur les résultats d'une expression régulière, dans
RuleRight::executeActions() à la ligne 164 de inc/ruleright.class.php,
suggère qu'il est possible d'affecter plusieurs entités avec une seule
règle.
Or je n'ai pas réussi à trouver comment configurer une règle permettant ce
type de résultat puisque l'interface ne permet qu'une seule affectation
d'entité par regexp.
De plus, les résultats des critères regexp sont combinées (cf. ligne 1144
dans inc/rule.class.php) en un seul tableau et le foreach ne fera qu'une
seule passe peu importe le nombre de critères regexp utilisés.
.Exemple de règle:
==================
Logical Operator: AND
Criteria:
- (LDAP)Distinguished Name | regular expression matches | /(OU=Développeurs)/
- (LDAP)Distinguished Name | regular expression matches | /(OU=Laboratoire)/
Actions:
- Entity based on LDAP information | Assign value from regexp | #0
Input:
[userdn] => CN=Kevin Roy,OU=Développeurs,OU=Laboratoire,DC=teclib,DC=info
Result:
[RuleRight::executeActions()] => Array
(
[this->regex_results] => Array
(
[0] => Array
(
[0] => OU=Développeurs
[1] => OU=Laboratoire
)
)
)
==================
J'ai donc remplacé cette boucle par une simple condition en me basant sur le
cas "regex_result" de la méthode Rule::executeActions() qui traite la
variable regex_results de cette même manière et qui est la plus récente
source de code traitant de ce sujet.
Merci d'avance pour vos futurs retours sur ce patch,
Cheers,
--
Kevin 'kiniou' Roy
From 00ac8dabe4616d33801b35c5290cdbfd55a2fa1a Mon Sep 17 00:00:00 2001
From: Kevin Roy <kin...@gmail.com>
Date: Fri, 28 Jun 2013 14:31:12 +0200
Subject: [PATCH] fix (and clean) ruleright code
On import or synchronization, some rules were doing wrong entities assignment.
In order to solve this issue and also to make the code understandable for further contribution, I've
tried to simplify the different parts i've managed to understand.
---
inc/ruleright.class.php | 112 +++++++++++++++++++-----------------------------
inc/user.class.php | 91 +++++++++++++++------------------------
2 files changed, 78 insertions(+), 125 deletions(-)
diff --git a/inc/ruleright.class.php b/inc/ruleright.class.php
index 6e7f7f3..f3e2de3 100644
--- a/inc/ruleright.class.php
+++ b/inc/ruleright.class.php
@@ -119,10 +119,9 @@ class RuleRight extends Rule {
function executeActions($output, $params) {
global $CFG_GLPI;
- $entity = '';
- $right = '';
+ $entity = array();
+ $profile = 0;
$is_recursive = 0;
- $continue = true;
$output_src = $output;
if (count($this->actions)) {
@@ -132,7 +131,7 @@ class RuleRight extends Rule {
case "assign" :
switch ($action->fields["field"]) {
case "entities_id" :
- $entity = $action->fields["value"];
+ $entity[] = $action->fields["value"];
break;
case "profiles_id" :
@@ -155,80 +154,55 @@ class RuleRight extends Rule {
break;
case "regex_result" :
- switch ($action->fields["field"]) {
- case "_affect_entity_by_dn" :
- case "_affect_entity_by_tag" :
- case "_affect_entity_by_domain" :
- case "_affect_entity_by_completename" :
- $entity = array();
- foreach ($this->regex_results as $regex_result) {
- $res = RuleAction::getRegexResultById($action->fields["value"],
- $regex_result);
- if ($res != null) {
- switch ($action->fields["field"]) {
- case "_affect_entity_by_dn" :
- $entity_found = Entity::getEntityIDByDN($res);
- break;
-
- case "_affect_entity_by_tag" :
- $entity_found = Entity::getEntityIDByTag($res);
- break;
-
- case "_affect_entity_by_domain" :
- $entity_found = Entity::getEntityIDByDomain($res);
- break;
-
- case "_affect_entity_by_completename" :
- $res = Toolbox::unclean_cross_side_scripting_deep($res);
- $entity_found = Entity::getEntityIDByCompletename($res);
- break;
-
- default:
- $entity_found = -1;
- break;
- }
-
- //If an entity was found
- if ($entity_found > -1) {
- $entity[] = $entity_found;
- }
- }
+ $entity = array();
+ if (isset($this->regex_results[0]) ) {
+ $regex_result = $this->regex_results[0];
+ $res = RuleAction::getRegexResultById($action->fields["value"],
+ $regex_result);
+ if ($res != null) {
+ switch ($action->fields["field"]) {
+ case "_affect_entity_by_dn" :
+ $entity_found = Entity::getEntityIDByDN($res);
+ break;
+
+ case "_affect_entity_by_tag" :
+ $entity_found = Entity::getEntityIDByTag($res);
+ break;
+
+ case "_affect_entity_by_domain" :
+ $entity_found = Entity::getEntityIDByDomain($res);
+ break;
+
+ case "_affect_entity_by_completename" :
+ $res = Toolbox::unclean_cross_side_scripting_deep($res);
+ $entity_found = Entity::getEntityIDByCompletename($res);
+ break;
+
+ default:
+ $entity_found = -1;
+ break;
}
- if (!count($entity)) {
- //Not entity assigned : action processing must be stopped for this rule
- $continue = false;
+ //If an entity was found
+ if ($entity_found > -1) {
+ $entity[] = $entity_found;
}
- break;
- } // switch (field)
+ }
+ }
break;
} // switch (action_type)
- } // foreach (action)
- } // count (actions)
- if ($continue) {
- //Nothing to be returned by the function :
- //Store in session the entity and/or right
- if (($entity != '')
- && ($right != '')) {
- $output["_ldap_rules"]["rules_entities_rights"][] = array($entity, $right,
- $is_recursive);
- } else if ($entity != '') {
-
- if (!is_array($entity)) {
- $entities_array = array($entity, $is_recursive);
- $output["_ldap_rules"]["rules_entities"][] = array($entities_array);
-
- //If it comes from a regex with multiple results
- } else {
- $output["_ldap_rules"]["rules_entities"][] = $entity;
- }
+ } // foreach (action)
- } else if ($right != '') {
- $output["_ldap_rules"]["rules_rights"][] = $right;
- }
+ } // count (actions)
+ if (count($entity)> 0) {
+ $output["_ldap_rules"]["results"][] = array(
+ "entities_id" => $entity,
+ "is_recursive" => $is_recursive,
+ "profiles_id" => $right
+ );
return $output;
}
return $output_src;
diff --git a/inc/user.class.php b/inc/user.class.php
index a2d08d7..15d8292 100644
--- a/inc/user.class.php
+++ b/inc/user.class.php
@@ -759,75 +759,54 @@ class User extends CommonDBTM {
&& count($this->input["_ldap_rules"])) {
//and add/update/delete only if it's necessary !
- if (isset($this->input["_ldap_rules"]["rules_entities_rights"])) {
- $entities_rules = $this->input["_ldap_rules"]["rules_entities_rights"];
+ if (isset($this->input["_ldap_rules"]["results"])) {
+ $rules_results = $this->input["_ldap_rules"]["results"];
} else {
- $entities_rules = array();
- }
-
- if (isset($this->input["_ldap_rules"]["rules_entities"])) {
- $entities = $this->input["_ldap_rules"]["rules_entities"];
- } else {
- $entities = array();
- }
-
- if (isset($this->input["_ldap_rules"]["rules_rights"])) {
- $rights = $this->input["_ldap_rules"]["rules_rights"];
- } else {
- $rights = array();
+ $rules_results = array();
}
$retrieved_dynamic_profiles = array();
- //For each affectation -> write it in DB
- foreach ($entities_rules as $entity) {
- //Multiple entities assignation
- if (is_array($entity[0])) {
- foreach ($entity[0] as $tmp => $ent) {
- $affectation['entities_id'] = $ent;
- $affectation['profiles_id'] = $entity[1];
- $affectation['is_recursive'] = $entity[2];
- $affectation['users_id'] = $this->fields['id'];
- $affectation['is_dynamic'] = 1;
-
- $retrieved_dynamic_profiles[] = $affectation;
- }
- } else {
- $affectation['entities_id'] = $entity[0];
- $affectation['profiles_id'] = $entity[1];
- $affectation['is_recursive'] = $entity[2];
- $affectation['users_id'] = $this->fields['id'];
- $affectation['is_dynamic'] = 1;
+ foreach ( $rules_results as $result ) {
- $retrieved_dynamic_profiles[] = $affectation;
+ $profiles_id = 0;
+ //Get Profile from action if any
+ if ( $result['profiles_id'] > 0 ) {
+ $profiles_id = $result['profiles_id'];
+ } else {
+ //If no profile from action, retrieve the default profile
+ if ( $profile_def = Profile::getDefault() ) {
+ $profiles_id = $profile_def;
+ };
}
- }
- if ((count($entities) > 0)
- && (count($rights) == 0)) {
- if ($def_prof = Profile::getDefault()) {
- $rights[] = $def_prof;
+ //If there are neither entities nor profiles, we continue the loop to the next
+ //action.
+ if( count($result['entities_id']) == 0
+ or $profiles_id == 0 ) {
+ continue;
}
- }
- if ((count($rights) > 0)
- && (count($entities) > 0)) {
- foreach ($rights as $right) {
- foreach ($entities as $entity_tab) {
- foreach ($entity_tab as $entity) {
- $affectation['entities_id'] = $entity[0];
- $affectation['profiles_id'] = $right;
- $affectation['users_id'] = $this->fields['id'];
- $affectation['is_recursive'] = $entity[1];
- $affectation['is_dynamic'] = 1;
-
- $retrieved_dynamic_profiles[] = $affectation;
- }
- }
+ /*****
+ * We loop through 'entities_id' list returned by RuleRight::executeActions()
+ * ... though IMHO there can only be one entity affected by one rule at a time
+ * and therefore we shouldn't use a foreach loop.
+ * -- Kevin 'kiniou' Roy
+ *****/
+
+ foreach ($result['entities_id'] as $entities_id) {
+
+ $assignment = array();
+ $assignment['users_id'] = $this->fields['id'];
+ $assignment['is_dynamic'] = 1;
+ $assignment['is_recursive'] = $result['is_recursive'];
+ $assignment['profiles_id'] = $profiles_id;
+ $assignment["entities_id"] = $entities_id;
+ $retrieved_dynamic_profiles[] = $assignment;
}
}
- // Compare retrived profiles to existing ones : clean arrays to do purge and add
+ // Compare retrieved profiles to existing ones : clean arrays to do purge and add
if (count($retrieved_dynamic_profiles)) {
foreach ($retrieved_dynamic_profiles as $keyretr => $retr_profile) {
$found = false;
--
1.8.3.1
_______________________________________________
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev
