Fixes #12808: Rudder API for ACL is buggy

rudder-core/src/main/scala/com/normation/rudder/api/ApiAccountDiff.scala

@@ -59,4 +59,5 @@ final case class ModifyApiAccountDiff(
   , modTokenGenerationDate : Option[SimpleDiff[DateTime]] = None
   , modExpirationDate      : Option[SimpleDiff[Option[DateTime]]] = None
   , modAccountKind         : Option[SimpleDiff[String]] = None
+  , modAccountAcl          : Option[SimpleDiff[List[ApiAclElement]]] = None
 ) extends ApiAccountDiff with HashcodeCaching

rudder-core/src/main/scala/com/normation/rudder/api/DataStructures.scala

@@ -136,7 +136,7 @@ final object AclPath {
   final case class FullPath(segments: NonEmptyList[AclPathSegment]) extends AclPath {
     def parts = segments
   }
-  // only the root is given, and the path ends with "**". It can even be onlyl "**"
+  // only the root is given, and the path ends with "**". It can even be only "**"
   final case class Root(segments: List[AclPathSegment])              extends AclPath {
     def parts = NonEmptyList.ofInitLast(segments, AclPathSegment.DoubleWildcard)
   }

rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPDiffMapper.scala

@@ -440,7 +440,22 @@ class LDAPDiffMapper(
                       kind.kind.name
                   }
                   Full(diff.copy(modAccountKind = Some(SimpleDiff(oldAuthType, mod.getAttribute().getValue()))))
+                case A_API_ACL =>
+                  val oldAcl = oldAccount.kind match {
+                    case PublicApi(ApiAuthorization.ACL(acl), _) =>
+                      acl
+                    case kind =>
+                      Nil
+                  }
 
+                  for {
+                    acl <- mapper.unserApiAcl(mod.getAttribute().getValue) match {
+                             case Left(error) => Failure(error)
+                             case Right(acl)  => Full(acl)
+                           }
+                  } yield {
+                    diff.copy(modAccountAcl = Some(SimpleDiff(oldAcl, acl)))
+                  }
 
                 case x => Failure("Unknown diff attribute: " + x)
               }

rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPEntityMapper.scala

@@ -792,7 +792,7 @@ class LDAPEntityMapper(
     ))
     write[JsonApiAcl](toSerialize)
   }
-  def unserApiAuthz(s: String): Either[String, List[ApiAclElement]] = {
+  def unserApiAcl(s: String): Either[String, List[ApiAclElement]] = {
     import cats.implicits._
     import net.liftweb.json._
     implicit val formats = net.liftweb.json.DefaultFormats
@@ -844,7 +844,7 @@ class LDAPEntityMapper(
                            case None    =>
                              logger.debug(s"API authorizations level kind for token '${name.value}' with id '${id.value}' is 'ACL' but it doesn't have any ACLs conigured")
                              Full(ApiAuthorization.None) // for Rudder < 4.3, it should have been migrated. So here, we just don't gave any access.
-                           case Some(s) => unserApiAuthz(s) match {
+                           case Some(s) => unserApiAcl(s) match {
                              case Right(x)  => Full(ApiAuthorization.ACL(x))
                              case Left(msg) => Failure(msg)
                            }

rudder-core/src/main/scala/com/normation/rudder/services/eventlog/EventLogDetailsService.scala

@@ -43,14 +43,12 @@ import net.liftweb.common.Box._
 import net.liftweb.util.Helpers.tryo
 import org.joda.time.format.ISODateTimeFormat
 import org.eclipse.jgit.lib.PersonIdent
-
 import com.normation.cfclerk.domain.TechniqueVersion
 import com.normation.cfclerk.domain.TechniqueName
 import com.normation.cfclerk.domain.TechniqueId
 import com.normation.inventory.domain.NodeId
 import com.normation.utils.Control.sequence
 import com.normation.eventlog.ModificationId
-
 import com.normation.rudder.api._
 import com.normation.rudder.batch.CurrentDeploymentStatus
 import com.normation.rudder.domain.policies._
@@ -71,6 +69,7 @@ import com.normation.rudder.services.queries.CmdbQueryParser
 import com.normation.rudder.services.marshalling._
 import com.normation.rudder.services.marshalling.TestFileFormat
 import net.liftweb.json.JsonAST.JString
+import org.joda.time.DateTime
 
 /**
  * A service that helps mapping event log details to there structured data model.
@@ -826,6 +825,8 @@ class EventLogDetailsServiceImpl(
   }
 
   def getApiAccountModifyDetails(xml:NodeSeq) : Box[ModifyApiAccountDiff] = {
+    import cats.implicits._
+
     for {
       entry              <- getEntryContent(xml)
       apiAccount         <- (entry \ XML_TAG_API_ACCOUNT).headOption ?~!
@@ -835,7 +836,25 @@ class EventLogDetailsServiceImpl(
       modToken           <- getFromToString((apiAccount \ "token").headOption)
       modDescription     <- getFromToString((apiAccount \ "description").headOption)
       modIsEnabled       <- getFromTo[Boolean]((apiAccount \ "enabled").headOption,
-                             { s => tryo { s.text.toBoolean } } )
+                              { s => tryo { s.text.toBoolean } } )
+      modTokenGenDate    <- getFromTo[DateTime]((apiAccount \ "tokenGenerationDate").headOption,
+                              { s => tryo { ISODateTimeFormat.dateTimeParser().parseDateTime(s.text) }} )
+      modExpirationDate  <- getFromTo[Option[DateTime]]((apiAccount \ "expirationDate").headOption,
+                              { s => Full(tryo { ISODateTimeFormat.dateTimeParser().parseDateTime(s.text) }.toOption) } )
+      modAccountKind     <- getFromToString((apiAccount \ "accountKind").headOption)
+      modAcls            <- getFromTo[List[ApiAclElement]]((apiAccount \ "acls").headOption,
+                              { s =>  ((s \ "acl").toList.traverse{x =>
+                                for {
+                                    path    <- AclPath.parse((x \ "@path").head.text)
+                                    actions <- (x \ "@actions").head.text.split(",").toList.traverse(HttpAction.parse _)
+                                } yield {
+                                    ApiAclElement(path, actions.toSet)
+                                }
+                              }) match {
+                                case Left(e)  => Failure(e)
+                                case Right(x) => Full(x)
+                              }
+                              } )
       fileFormatOk       <- TestFileFormat(apiAccount)
     } yield {
       ModifyApiAccountDiff(
@@ -844,6 +863,10 @@ class EventLogDetailsServiceImpl(
         , modToken = modToken
         , modDescription = modDescription
         , modIsEnabled = modIsEnabled
+        , modTokenGenerationDate = modTokenGenDate
+        , modExpirationDate      = modExpirationDate
+        , modAccountKind         = modAccountKind
+        , modAccountAcl          = modAcls
       )
     }
   }

rudder-core/src/main/scala/com/normation/rudder/services/eventlog/EventLogFactory.scala

@@ -39,9 +39,7 @@ package com.normation.rudder.services.eventlog
 
 import scala.xml._
 import scala.xml.Text
-
 import org.joda.time.DateTime
-
 import com.normation.cfclerk.domain.SectionSpec
 import com.normation.cfclerk.domain.TechniqueVersion
 import com.normation.eventlog._
@@ -56,8 +54,8 @@ import com.normation.rudder.domain.policies._
 import com.normation.rudder.domain.queries.Query
 import com.normation.rudder.domain.workflows.WorkflowStepChange
 import com.normation.rudder.services.marshalling._
-
 import net.liftweb.util.Helpers._
+import org.joda.time.format.ISODateTimeFormat
 
 trait EventLogFactory {
 
@@ -780,7 +778,20 @@ class EventLogFactoryImpl(
           diff.modName.map(x => SimpleDiff.stringToXml(<name/>, x) ) ++
           diff.modToken.map(x => SimpleDiff.stringToXml(<token/>, x) ) ++
           diff.modDescription.map(x => SimpleDiff.stringToXml(<description/>, x ) ) ++
-          diff.modIsEnabled.map(x => SimpleDiff.booleanToXml(<enabled/>, x ) )
+          diff.modIsEnabled.map(x => SimpleDiff.booleanToXml(<enabled/>, x ) ) ++
+          diff.modTokenGenerationDate.map(x => SimpleDiff.toXml[DateTime](<tokenGenerationDate/>, x){ x =>
+            Text(x.toString(ISODateTimeFormat.dateTime()))
+          })++
+          diff.modExpirationDate.map(x => SimpleDiff.toXml[Option[DateTime]](<expirationDate/>, x){ x =>
+            x match {
+              case None    => NodeSeq.Empty
+              case Some(d) => Text(d.toString(ISODateTimeFormat.dateTime()))
+            }
+          }) ++
+          diff.modAccountKind.map(x => SimpleDiff.stringToXml(<accountKind/>, x)) ++
+          diff.modAccountAcl.map(x => SimpleDiff.toXml[List[ApiAclElement]](<acls/>, x){x =>
+            x.map(acl => <acl path={acl.path.value} actions={acl.actions.map(_.name).mkString(",")} />)
+          })
         }
       </apiAccount>)
     }

rudder-core/src/test/scala/com/normation/rudder/hooks/RunNuCommandTest.scala

@@ -50,7 +50,7 @@ import scala.collection.JavaConverters._
  */
 
 @RunWith(classOf[JUnitRunner])
-case class RunNuCommandTest(implicit ee: ExecutionEnv) extends Specification {
+case class RunNuCommandTest()(implicit ee: ExecutionEnv) extends Specification {
 
   "A command" should {
 

rudder-rest/src/main/scala/com/normation/rudder/rest/RestDataSerializer.scala

@@ -561,23 +561,34 @@ case class RestDataSerializerImpl (
 
 }
 
+/*
+ * ACL
+ * Between front and backend, we exchange a JsonAcl list, where JsonAcl are *just*
+ * one path and one verb. The grouping is done in extractor
+ */
+final case class JsonApiAcl(path: String, verb: String)
+
 object ApiAccountSerialisation {
 
+  implicit val formats = Serialization.formats(NoTypeHints)
+
+
   implicit class Json(account: ApiAccount) {
 
-    val (expirationDate, authzType, aclList) :  (Option[String],Option[String],Option[List[String]]) = {
+    val (expirationDate, authzType, aclList) :  (Option[String],Option[String],Option[List[JsonApiAcl]]) = {
       account.kind match {
         case User | System => (None,None,None)
         case PublicApiAccount(authz,expirationDate) =>
           val aclList = authz match {
             case NoAccess | RO | RW => None
-            case ACL(acls) => Some(acls.map(_.path.value))
+            case ACL(acls) => Some(acls.flatMap(x => x.actions.map(a => JsonApiAcl(x.path.value, a.name))))
           }
           ( expirationDate.map(DateFormaterService.getFormatedDate)
           , Some(authz.kind.name)
           , aclList )
       }
     }
+
     def toJson(): JObject = {
       ("id"                    -> account.id.value) ~
       ("name"                  -> account.name.value) ~
@@ -590,8 +601,7 @@ object ApiAccountSerialisation {
       ("expirationDate"        -> expirationDate) ~
       ("expirationDateDefined" -> expirationDate.isDefined ) ~
       ("authorizationType"     -> authzType ) ~
-      ("aclList"               -> aclList)
+      ("aclList"               -> aclList.map(x => Extraction.decompose(x)))
     }
-
   }
 }

rudder-rest/src/main/scala/com/normation/rudder/rest/RestExtractorService.scala

@@ -828,19 +828,16 @@ case class RestExtractorService (
   }
 
 
-  def extractApiACLFromJSON (json : JValue) : Box[ApiAclElement] = {
+  /*
+   * The ACL list which is exchange between
+   */
+  def extractApiACLFromJSON (json : JValue) : Box[(AclPath, HttpAction)] = {
     import com.normation.rudder.utils.Utils.eitherToBox
-    def extractAPIVerb(json : JValue) : Box[HttpAction] = {
-      json match {
-        case JString(v) => HttpAction.parse(v)
-        case s => Failure(s"Not a valid value for an http verb, expected a String, got ${compactRender(s)}")
-      }
-    }
     for {
-      path <- CompleteJson.extractJsonString(json, "path", AclPath.parse)
-      actions <- CompleteJson.extractJsonArray(json \ "verbs")(extractAPIVerb)
+      path   <- CompleteJson.extractJsonString(json, "path", AclPath.parse)
+      action <- CompleteJson.extractJsonString(json, "verb", HttpAction.parse)
     } yield {
-      ApiAclElement(path, actions.toSet)
+      (path, action)
     }
   }
 
@@ -868,7 +865,13 @@ case class RestExtractorService (
         case Some(ApiAuthorizationKind.None) => Some(ApiAuthz.None)
         case Some(ApiAuthorizationKind.RO) => Some(ApiAuthz.RO)
         case Some(ApiAuthorizationKind.RW) => Some(ApiAuthz.RW)
-        case Some(ApiAuthorizationKind.ACL) => Some(ApiAuthz.ACL(aclList))
+        case Some(ApiAuthorizationKind.ACL) => {
+          //group by path to get ApiAclElements
+          val acls = aclList.groupBy(_._1).map { case (p, seq) =>
+            ApiAclElement(p, seq.map(_._2).toSet)
+          }.toList
+          Some(ApiAuthz.ACL(acls))
+        }
       }
       val expiration = expirationDefined match {
         case None => None

rudder-web/src/main/scala/com/normation/rudder/web/services/EventListDisplayer.scala

@@ -64,6 +64,7 @@ import org.eclipse.jgit.lib.PersonIdent
 import org.joda.time.DateTime
 import com.normation.rudder.services.eventlog.RollbackInfo
 import com.normation.rudder.domain.workflows.ChangeRequestId
+
 import scala.util.Try
 import scala.util.Success
 import scala.util.{Failure => Catch}
@@ -80,6 +81,7 @@ import com.normation.rudder.rule.category.RuleCategory
 import com.normation.rudder.rule.category.RoRuleCategoryRepository
 import org.joda.time.format.DateTimeFormat
 import com.normation.rudder.web.model.LinkUtil
+import org.joda.time.format.ISODateTimeFormat
 
 /**
  * Used to display the event list, in the pending modification (AsyncDeployment),
@@ -1027,6 +1029,8 @@ class EventListDisplayer(
       case mod:ModifyAPIAccountEventLog =>
         "*" #> { logDetailsService.getApiAccountModifyDetails(mod.details) match {
           case Full(apiAccountDiff) =>
+
+            println(("**** " + apiAccountDiff))
               <div class="evloglmargin">
               { addRestoreAction }
               { generatedByChangeRequest }
@@ -1038,7 +1042,16 @@ class EventListDisplayer(
                   "#name" #> mapSimpleDiff(apiAccountDiff.modName) &
                   "#token" #>  mapSimpleDiff(apiAccountDiff.modToken) &
                   "#description *" #> mapSimpleDiff(apiAccountDiff.modDescription) &
-                  "#isEnabled *" #> mapSimpleDiff(apiAccountDiff.modIsEnabled)
+                  "#isEnabled *" #> mapSimpleDiff(apiAccountDiff.modIsEnabled) &
+                  "#tokenGenerationDate *" #> mapSimpleDiff(apiAccountDiff.modTokenGenerationDate) &
+                  "#expirationDate *" #> mapSimpleDiffT[Option[DateTime]](apiAccountDiff.modExpirationDate, _.fold("")(_.toString(ISODateTimeFormat.dateTime()))
+                  ) &
+                  "#accountKind *" #> mapSimpleDiff(apiAccountDiff.modAccountKind) &
+                  //make list of ACL unsderstandable
+                  "#acls *" #> mapSimpleDiff(apiAccountDiff.modAccountAcl.map(o => {
+                                 val f = (l: List[ApiAclElement]) => l.sortBy(_.path.value).map(x => s"[${x.actions.map(_.name.toUpperCase()).mkString(",")}] ${x.path.value}").mkString(" | ")
+                                 SimpleDiff(f(o.oldValue), f(o.newValue))
+                               }))
                 )(apiAccountModDetailsXML)
                 }
               { reasonHtml }
@@ -1319,11 +1332,14 @@ class EventListDisplayer(
       "#tokenGenerationDate" #> DateFormaterService.getFormatedDate(apiAccount.tokenGenerationDate)
   )(xml)
 
- private[this] def mapSimpleDiff[T](opt:Option[SimpleDiff[T]]) = opt.map { diff =>
-   ".diffOldValue *" #> diff.oldValue.toString &
-   ".diffNewValue *" #> diff.newValue.toString
+
+ private[this] def mapSimpleDiffT[T](opt:Option[SimpleDiff[T]], t: T => String) = opt.map { diff =>
+   ".diffOldValue *" #> t(diff.oldValue) &
+   ".diffNewValue *" #> t(diff.newValue)
   }
 
+ private[this] def mapSimpleDiff[T](opt:Option[SimpleDiff[T]]) = mapSimpleDiffT(opt, (x:T) => x.toString)
+
  private[this] def mapSimpleDiff[T](opt:Option[SimpleDiff[T]], id: DirectiveId) = opt.map { diff =>
    ".diffOldValue *" #> diff.oldValue.toString &
    ".diffNewValue *" #> diff.newValue.toString &
@@ -1439,9 +1455,25 @@ class EventListDisplayer(
         <li><b>Enabled:&nbsp;</b><value id="isEnabled"/></li>
         <li><b>Creation date:&nbsp;</b><value id="creationDate"/></li>
         <li><b>Token Generation date:&nbsp;</b><value id="tokenGenerationDate"/></li>
+        <li><b>Token Expiration date:&nbsp;</b><value id="expirationDate"/></li>
+        <li><b>Account Kind:&nbsp;</b><value id="accountKind"/></li>
+        <li><b>ACLs:&nbsp;</b><value id="acls"/></li>
       </ul>
     </div>
 
+  private[this] val apiAccountModDetailsXML =
+    <xml:group>
+      {liModDetailsXML("name", "Name")}
+      {liModDetailsXML("token", "Token")}
+      {liModDetailsXML("description", "Description")}
+      {liModDetailsXML("isEnabled", "Enabled")}
+      {liModDetailsXML("tokenGenerationDate", "Token Generation Date")}
+      {liModDetailsXML("expirationDate", "Token Expiration Date")}
+      {liModDetailsXML("accountKind", "Account Kind")}
+      {liModDetailsXML("acls", "ACL list")}
+    </xml:group>
+
+
   private[this] def liModDetailsXML(id:String, name:String) = (
       <div id={id}>
         <b>{name} changed:</b>
@@ -1504,13 +1536,6 @@ class EventListDisplayer(
       {liModDetailsXML("overridable", "Overridable")}
     </xml:group>
 
-  private[this] val apiAccountModDetailsXML =
-    <xml:group>
-      {liModDetailsXML("name", "Name")}
-      {liModDetailsXML("token", "Token")}
-      {liModDetailsXML("description", "Description")}
-      {liModDetailsXML("isEnabled", "Enabled")}
-    </xml:group>
 
   private[this] def displayRollbackDetails(rollbackInfo:RollbackInfo,id:Int) = {
     val rollbackedEvents = rollbackInfo.rollbacked