Skip to content
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.

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

Fixes #3286: system items can not be modified like non-system ones #140

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,20 @@ trait ConfigurationRuleRepository {
* parameters.
*
* If the configuration rule is not in the repos, the method fails.
* If the configuration rule is a system one, the methods fails.
*
* NOTE: the serial is *never* updated with that methods.
*/
def update(cr:ConfigurationRule, actor:EventActor) : Box[Option[ModifyConfigurationRuleDiff]]

/**
* Update the system configuration rule with the given ID with the given
* parameters.
*
* NOTE: the serial is *never* updated with that methods.
*/
def updateSystem(cr:ConfigurationRule, actor:EventActor) : Box[Option[ModifyConfigurationRuleDiff]]

/**
* Increment the serial of Configuration Rules with given ID by one.
* Return the new serial value.
Expand All @@ -100,6 +109,8 @@ trait ConfigurationRuleRepository {
* If no configuration rule with such ID exists, it is an error
* (it's the caller site responsability to decide if it's
* and error or not).
*
* A system rule can not be deleted.
*/
def delete(id:ConfigurationRuleId, actor:EventActor) : Box[DeleteConfigurationRuleDiff]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,15 @@ trait NodeGroupRepository {
* That method does nothing at the configuration level,
* so you will have to manage configuration rule deployment
* if needed
*
* System group can not be updated with that method.
*/
def update(group:NodeGroup, actor:EventActor) : Box[Option[ModifyNodeGroupDiff]]

/**
* Update the given existing system group
*/
def updateSystemGroup(group:NodeGroup, actor:EventActor) : Box[Option[ModifyNodeGroupDiff]]

/**
* Move the given existing group to the new container.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,26 @@ trait PolicyInstanceRepository {
* update the policy instance.
* If the policy instance is not in the system, add it.
*
* System policy instance can't be saved with that method.
*
* Returned the saved UserPolicyInstance
*/
def savePolicyInstance(inUserPolicyTemplateId:UserPolicyTemplateId,pi:PolicyInstance, actor:EventActor) : Box[Option[PolicyInstanceSaveDiff]]

/**
* Save the given system policy instance into given user policy template
* If the policy instance is already present in the system but not
* in the given category, raise an error.
* If the policy instance is already in the given policy template,
* update the policy instance.
* If the policy instance is not in the system, add it.
*
* Non system policy instance can't be saved with that method.
*
* Returned the saved UserPolicyInstance
*/
def saveSystemPolicyInstance(inUserPolicyTemplateId:UserPolicyTemplateId,pi:PolicyInstance, actor:EventActor) : Box[Option[PolicyInstanceSaveDiff]]

/**
* Get all policy instances defined in that repository
*/
Expand All @@ -98,6 +114,8 @@ trait PolicyInstanceRepository {
*
* If the given policyInstanceId does not exists, it leads to a
* failure.
*
* System policy instance can't be deleted.
*/
def delete(id:PolicyInstanceId, actor:EventActor) : Box[DeletePolicyInstanceDiff]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ class LDAPConfigurationRuleRepository(
con <- ldap
entry <- con.get(rudderDit.CONFIG_RULE.configRuleDN(id.value)) ?~! "Configuration rule with ID '%s' is not present".format(id.value)
oldCr <- mapper.entry2ConfigurationRule(entry) ?~! "Error when transforming LDAP entry into a Configuration Rule for id %s. Entry: %s".format(id, entry)
isSytem <- if(oldCr.isSystem) Failure("Deleting system rule '%s (%s)' is forbiden".format(oldCr.name, oldCr.id.value)) else Full("OK")
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a typo to me - not sure if it has any impact, but still, I though it was worth mentioning, since everywhere else this is "isSystem".

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, even if it does not have any impact, since it's a fresh variable name here.

deleted <- con.delete(rudderDit.CONFIG_RULE.configRuleDN(id.value)) ?~! "Error when deleting configuration rule with ID %s".format(id)
diff = DeleteConfigurationRuleDiff(oldCr)
loggedAction <- actionLogger.saveEventLog(DeleteConfigurationRule.fromDiff(principal = actor, deleteDiff = diff))
Expand Down Expand Up @@ -134,10 +135,16 @@ class LDAPConfigurationRuleRepository(
}


def update(cr:ConfigurationRule, actor:EventActor) : Box[Option[ModifyConfigurationRuleDiff]] = {
private[this] def internalUpdate(cr:ConfigurationRule, actor:EventActor, systemCall:Boolean) : Box[Option[ModifyConfigurationRuleDiff]] = {
repo.synchronized { for {
con <- ldap
existingEntry <- con.get(rudderDit.CONFIG_RULE.configRuleDN(cr.id.value)) ?~! "Cannot update configuration rule with id %s : there is no configuration rule with that id".format(cr.id.value)
oldCr <- mapper.entry2ConfigurationRule(existingEntry) ?~! "Error when transforming LDAP entry into a Configuration Rule for id %s. Entry: %s".format(cr.id, existingEntry)
systemCheck <- (oldCr.isSystem, systemCall) match {
case (true, false) => Failure("System rule '%s' (%s) can not be modified".format(oldCr.name, oldCr.id.value))
case (false, true) => Failure("You can't modify a non-system rule with updateSystem method")
case _ => Full("OK")
}
crEntry = mapper.configurationRule2Entry(cr)
result <- con.save(crEntry, true, Seq(A_SERIAL)) ?~! "Error when saving Configuration Rule entry in repository: %s".format(crEntry)
optDiff <- diffMapper.modChangeRecords2ConfigurationRuleDiff(existingEntry,result) ?~! "Error when mapping Configuration Rule '%s' update to an diff: %s".format(cr.id.value, result)
Expand All @@ -150,6 +157,13 @@ class LDAPConfigurationRuleRepository(
} }
}

def update(cr:ConfigurationRule, actor:EventActor) : Box[Option[ModifyConfigurationRuleDiff]] = {
internalUpdate(cr, actor, false)
}

def updateSystem(cr:ConfigurationRule, actor:EventActor) : Box[Option[ModifyConfigurationRuleDiff]] = {
internalUpdate(cr, actor, true)
}

def incrementSerial(id:ConfigurationRuleId) : Box[Int] = {
repo.synchronized { for {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,17 @@ class LDAPNodeGroupRepository(
} }
}

def update(nodeGroup:NodeGroup, actor:EventActor): Box[Option[ModifyNodeGroupDiff]] = {
private[this] def internalUpdate(nodeGroup:NodeGroup, actor:EventActor, systemCall:Boolean): Box[Option[ModifyNodeGroupDiff]] = {
repo.synchronized {
for {
con <- ldap
existing <- getSGEntry(con, nodeGroup.id) ?~! "Error when trying to check for existence of group with id %s. Can not update".format(nodeGroup.id)
oldGroup <- mapper.entry2NodeGroup(existing) ?~! "Error when trying to check for the group %s".format(nodeGroup.id.value)
systemCheck <- (oldGroup.isSystem, systemCall) match {
case (true, false) => Failure("System group '%s' (%s) can not be modified".format(oldGroup.name, oldGroup.id.value))
case (false, true) => Failure("You can not modify a non system group (%s) with that method".format(oldGroup.name))
case _ => Full(oldGroup)
}
exists <- if (nodeGroupExists(con, nodeGroup.name, nodeGroup.id)) Failure("Cannot change the group name to %s : there is already a group with the same name".format(nodeGroup.name))
else Full(Unit)
entry = rudderDit.GROUP.groupModel(
Expand All @@ -188,11 +194,22 @@ class LDAPNodeGroupRepository(
} }
}

def update(group:NodeGroup, actor:EventActor) : Box[Option[ModifyNodeGroupDiff]] = {
internalUpdate(group, actor, false)
}


def updateSystemGroup(group:NodeGroup, actor:EventActor) : Box[Option[ModifyNodeGroupDiff]] = {
internalUpdate(group, actor, true)
}

def move(nodeGroup:NodeGroup, containerId : NodeGroupCategoryId, actor:EventActor): Box[Option[ModifyNodeGroupDiff]] = {
repo.synchronized {
for {
con <- ldap
existing <- getSGEntry(con, nodeGroup.id) ?~! "Error when trying to check for existence of group with id %s. Can not update".format(nodeGroup.id)
oldGroup <- mapper.entry2NodeGroup(existing) ?~! "Error when trying to get the existing group with id %s".format(nodeGroup.id.value)
systemCheck <- if(oldGroup.isSystem) Failure("You can not move system group") else Full("OK")
groupRDN <- Box(existing.rdn) ?~! "Error when retrieving RDN for an exising group - seems like a bug"
exists <- if (nodeGroupExists(con, nodeGroup.name, nodeGroup.id)) Failure("Cannot change the group name to %s : there is already a group with the same name".format(nodeGroup.name))
else Full(Unit)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ class LDAPPolicyInstanceRepository(
*
* Returned the saved WBUserPolicyInstance
*/
def savePolicyInstance(inUserPolicyTemplateId:UserPolicyTemplateId,pi:PolicyInstance, actor:EventActor) : Box[Option[PolicyInstanceSaveDiff]] = {
private[this] def internalSavePolicyInstance(inUserPolicyTemplateId:UserPolicyTemplateId,pi:PolicyInstance, actor:EventActor, systemCall:Boolean) : Box[Option[PolicyInstanceSaveDiff]] = {
repo.synchronized { for {
con <- ldap
uptEntry <- ldapUserPolicyTemplateRepository.getUPTEntry(con, inUserPolicyTemplateId, "1.1") ?~! "Can not find the User Policy Entry with id %s to add Policy Instance %s".format(inUserPolicyTemplateId, pi.id)
Expand All @@ -162,7 +162,15 @@ class LDAPPolicyInstanceRepository(
case f:Failure => f
case Empty => Full(None)
case Full(otherPi) =>
if(otherPi.dn.getParent == uptEntry.dn) Full(Some(otherPi))
if(otherPi.dn.getParent == uptEntry.dn) {
mapper.entry2PolicyInstance(otherPi).flatMap { x =>
(x.isSystem, systemCall) match {
case (true, false) => Failure("System policy instance '%s' (%s) can't be updated".format(x.name, x.id.value))
case (false, true) => Failure("Non-system policy instance can not be updated with that method")
case _ => Full(Some(otherPi))
}
}
}
else Failure("An other policy instance with the id %s exists in an other category that the one with id %s : %s".format(pi.id, inUserPolicyTemplateId, otherPi.dn))
}
}
Expand All @@ -187,6 +195,13 @@ class LDAPPolicyInstanceRepository(
} }
}

def savePolicyInstance(inUserPolicyTemplateId:UserPolicyTemplateId,pi:PolicyInstance, actor:EventActor) : Box[Option[PolicyInstanceSaveDiff]] = {
internalSavePolicyInstance(inUserPolicyTemplateId, pi, actor, false)
}

def saveSystemPolicyInstance(inUserPolicyTemplateId:UserPolicyTemplateId,pi:PolicyInstance, actor:EventActor) : Box[Option[PolicyInstanceSaveDiff]] = {
internalSavePolicyInstance(inUserPolicyTemplateId, pi, actor, true)
}

/**
* Delete a policy instance.
Expand All @@ -200,6 +215,7 @@ class LDAPPolicyInstanceRepository(
entry <- getPolicyInstanceEntry(con, id)
//for logging, before deletion
pi <- mapper.entry2PolicyInstance(entry)
systemCheck <- if(pi.isSystem) Failure("System policy instance '%s' (%s) can't be deleted".format(pi.name, pi.id.value)) else Full("OK")
upt <- this.getUserPolicyTemplate(id) ?~! "Can not find the User Policy Temple Entry for Policy Instance %s".format(id)
pt <- policyPackageService.getPolicy(PolicyPackageId(upt.referencePolicyTemplateName,pi.policyTemplateVersion))
//delete
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ trait DynGroupUpdaterService {
/**
* Update the given dynamic group, returning the diff
* from the pre-update.
*
* IMPORTANT NOTE: system group are not updated with
* that service !
*
* @return
*/
def update(dynGroupId:NodeGroupId, actor:EventActor) : Box[DynGroupDiff]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ class AcceptServerConfigurationRule(
for {
group <- groupRepo.getNodeGroup(hasPolicyServerNodeGroup) ?~! "Technical group with ID '%s' was not found".format(hasPolicyServerNodeGroup)
updatedGroup = group.copy( serverList = group.serverList + nodeId )
saved<- groupRepo.update(updatedGroup,actor) ?~! "Could not update the technical group with ID '%s'".format(updatedGroup.id )
saved<- groupRepo.updateSystemGroup(updatedGroup,actor) ?~! "Could not update the technical group with ID '%s'".format(updatedGroup.id )
} yield {
nodeId
}
Expand All @@ -658,7 +658,7 @@ class AcceptServerConfigurationRule(
(for {
group <- groupRepo.getNodeGroup(buildHasPolicyServerGroupId(sm.node.main.policyServerId)) ?~! "Can not find group with id: %s".format(sm.node.main.policyServerId)
updatedGroup = group.copy( serverList = group.serverList.filter(x => x != sm.node.main.id ) )
saved<- groupRepo.update(updatedGroup, actor)?~! "Error when trying to update dynamic group %s with member %s".format(updatedGroup.id,sm.node.main.id.value)
saved<- groupRepo.updateSystemGroup(updatedGroup, actor)?~! "Error when trying to update dynamic group %s with member %s".format(updatedGroup.id,sm.node.main.id.value)
} yield {
sm
}) match {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class PolicyServerManagementServiceImpl(
pi <- policyInstanceRepository.getPolicyInstance(piId) ?~! "Error when retrieving policy instance with ID '%s'".format(piId.value)
upt <- policyInstanceRepository.getUserPolicyTemplate(piId) ?~! "Error when getting user policy template for policy instance with ID '%s'".format(piId.value)
newPi = pi.copy(parameters = pi.parameters + (Constants.V_ALLOWED_NETWORK -> networks.map( _.toString)))
saved <- policyInstanceRepository.savePolicyInstance(upt.id, newPi, actor) ?~! "Can not save policy instance for User Policy Template '%s'".format(upt.id.value)
saved <- policyInstanceRepository.saveSystemPolicyInstance(upt.id, newPi, actor) ?~! "Can not save policy instance for User Policy Template '%s'".format(upt.id.value)
} yield {
//ask for a new policy deployment
asyncDeploymentAgent ! StartDeployment(RudderEventActor)
Expand Down