fix: keep the code-defined system views current on every installation, not only fresh ones (#80)

The UK Open Banking and Berlin Group views carry can_* sets defined in
Constant.SYSTEM_READ_*_VIEW_PERMISSION, and applyDefaultsForSystemView writes
them. It runs from exactly two places: unsavedSystemView, at creation, and
factoryResetSystemView, an admin endpoint. Boot calls getOrCreateSystemView,
which returns an existing row untouched.

So a view created by an older version keeps that version's permission set for
good. Tightening a set in code reaches new installations and no others -- which
is how "ReadAccountsDetail granted nothing beyond ReadAccountsBasic" and
"ReadBalances alone exposed transaction and counterparty data" could be fixed in
code and remain true of every upgraded database.

Boot now calls ensureSystemViewUpToDate, which creates the view when missing and
otherwise re-applies the code-defined permissions to the existing row. Only the
permission set: name, description, isPublic and the alias flags are left alone,
so operator edits to those survive an upgrade. Unlike a migration this is not
opt-in -- migration_scripts.execute_all defaults to false, so shipping this as a
migration would have reproduced the very shape of the defect, a fix that does not
apply to the installations that have the problem.

system_views.reconcile_permissions_at_boot turns it off for an operator who
hand-tunes these rows. Default true, and every view whose set moves is logged
with the permissions added and removed.

The nine views the code defines sets for are also created unconditionally rather
than through additional_system_views. The code already depends on them: a UK
consent naming ReadTransactionsCredits cannot be granted if that view is absent,
and validateUKConsentPermissions requires a direction permission whenever a
transaction-depth one is granted -- so a conforming consent could not be
exercised at all on an instance whose props predated the view.
ReadTransactionsBerlinGroup and InitiatePaymentsBerlinGroup stay opt-in, since
nothing requires them, but are reconciled where they exist: the prop decides
whether a view exists, not whether the code is authoritative about what it grants.

Written test-first. The new upgrade scenario fails before the change with
ReadAccountsBasic still carrying the 74-permission generic set -- including
can_see_transaction_amount and can_see_other_account_iban on a view meant to
expose four account fields. The scenario that was already there cannot catch
this: afterEach drops every ViewDefinition, so it only ever exercises the create
path.

MappedViewsTest 7/7, full local suite 3378/0 across all four shards.

Known gap, not closed here: the two Berlin Group views' real state on a
long-lived database is not established. One development database has
ReadAccountsBerlinGroup carrying 176 distinct permissions accumulated across
three separate dates, which is neither the generic set nor the target. The tests
cover create-fresh and upgrade-from-the-generic-set; upgrading from that third
shape is untested.

Author: hongwei1

obp-api/src/main/resources/props/sample.props.template

@@ -1293,18 +1293,35 @@ database_messages_scheduler_interval=3600
 # Create System Views At Boot -----------------------------------------------
 # In case is not defined default value is true
 # create_system_views_at_boot=true
+#
+# The UK Open Banking and Berlin Group views the code defines can_* sets for are created
+# unconditionally and no longer need listing here -- the code depends on them existing. A UK
+# consent naming ReadTransactionsCredits cannot be granted if that view is absent, and a
+# conforming consent is *required* to name a direction permission whenever it names a
+# transaction-depth one, so leaving their creation to a props file made conforming consents
+# unusable on instances whose props predated them:
+#   ReadAccountsBasic, ReadAccountsDetail, ReadBalances, ReadTransactionsBasic,
+#   ReadTransactionsDebits, ReadTransactionsCredits, ReadTransactionsDetail,
+#   ReadAccountsBerlinGroup, ReadBalancesBerlinGroup
+#
 # additional_system_views=
-# Possible values for additional_system_views are: ReadAccountsBasic,\
-                                                    ReadAccountsDetail,\
-                                                    ReadBalances,\
-                                                    ReadTransactionsBasic,\
-                                                    ReadTransactionsDebits,\
-                                                    ReadTransactionsCredits,\
-                                                    ReadTransactionsDetail, \
-                                                    ReadAccountsBerlinGroup, \
-                                                    ReadBalancesBerlinGroup, \
-                                                    ReadTransactionsBerlinGroup, \
+# Possible values for additional_system_views are: ReadTransactionsBerlinGroup, \
                                                     InitiatePaymentsBerlinGroup
+#
+# Whether boot brings existing system views' permissions back in line with what this build
+# defines. Default true.
+#
+# getOrCreateSystemView returns an existing row untouched, so before this a view created by an
+# older version kept that version's permission set for good: a set tightened in code reached new
+# installations and no others. That is how "ReadAccountsDetail granted nothing beyond
+# ReadAccountsBasic" and "ReadBalances alone exposed transaction and counterparty data" could be
+# fixed in code and still be true of every upgraded database.
+#
+# Only the permission set is reconciled -- a view's name, description, isPublic and alias flags
+# are left alone, so operator edits to those survive. Set to false only if you deliberately
+# hand-tune these rows and intend to keep them current yourself; every reconciliation is logged
+# with the permissions added and removed.
+# system_views.reconcile_permissions_at_boot=true
 # -----------------------------------------------------------------------------
 
 

obp-api/src/main/scala/bootstrap/liftweb/Boot.scala

@@ -317,27 +317,57 @@ class Boot extends MdcLoggable {
         Views.views.vend.getOrCreateSystemView(SYSTEM_FIREHOSE_VIEW_ID).isDefined
       else Empty.isDefined
 
+      // The UK Open Banking and Berlin Group views whose can_* sets the code defines
+      // (Constant.SYSTEM_READ_*_VIEW_PERMISSION). Ensured unconditionally rather than through
+      // additional_system_views: the code already depends on them existing. A UK consent naming
+      // ReadTransactionsCredits cannot be granted if that view is absent, and
+      // validateUKConsentPermissions *requires* a direction permission whenever a
+      // transaction-depth one is granted -- so a conforming consent could not be exercised on an
+      // instance whose props predated the view.
+      //
+      // ensureSystemViewUpToDate, not getOrCreateSystemView: the latter returns an existing row
+      // untouched, so a view created by an older version keeps that version's permission set for
+      // good and a tightening in code reaches new installations only. That is how "Detail granted
+      // nothing beyond Basic" and "Balances alone exposed transaction data" survived being fixed.
+      val codeDefinedSystemViews = List(
+        SYSTEM_READ_ACCOUNTS_BASIC_VIEW_ID,
+        SYSTEM_READ_ACCOUNTS_DETAIL_VIEW_ID,
+        SYSTEM_READ_BALANCES_VIEW_ID,
+        SYSTEM_READ_TRANSACTIONS_BASIC_VIEW_ID,
+        SYSTEM_READ_TRANSACTIONS_DEBITS_VIEW_ID,
+        SYSTEM_READ_TRANSACTIONS_CREDITS_VIEW_ID,
+        SYSTEM_READ_TRANSACTIONS_DETAIL_VIEW_ID,
+        SYSTEM_READ_ACCOUNTS_BERLIN_GROUP_VIEW_ID,
+        SYSTEM_READ_BALANCES_BERLIN_GROUP_VIEW_ID
+      )
+      // Default true: a permission set the code tightened must reach the installations that have
+      // the problem, not only fresh ones. An operator who has deliberately hand-tuned these rows
+      // turns it off and takes responsibility for keeping them current.
+      if (APIUtil.getPropsAsBoolValue("system_views.reconcile_permissions_at_boot", true)) {
+        codeDefinedSystemViews.foreach(Views.views.vend.ensureSystemViewUpToDate)
+      } else {
+        logger.warn("system_views.reconcile_permissions_at_boot is false: the UK Open Banking and " +
+          "Berlin Group system views keep whatever permissions they already carry, which may be " +
+          "an older and wider set than this build defines.")
+        codeDefinedSystemViews.foreach(Views.views.vend.getOrCreateSystemView)
+      }
+
+      // The remaining two stay opt-in: nothing in the code requires them to exist, so whether an
+      // instance has them is still the operator's call. Their permission sets ARE code-defined
+      // though, so where they do exist they are kept current the same way -- the prop decides
+      // existence, not whether the code is authoritative about what a view grants.
       APIUtil.getPropsValue("additional_system_views") match {
         case Full(value) =>
           val additionalSystemViewsFromProps = value.split(",").map(_.trim).toList
           val additionalSystemViews = List(
-            SYSTEM_READ_ACCOUNTS_BASIC_VIEW_ID,
-            SYSTEM_READ_ACCOUNTS_DETAIL_VIEW_ID,
-            SYSTEM_READ_BALANCES_VIEW_ID,
-            SYSTEM_READ_TRANSACTIONS_BASIC_VIEW_ID,
-            SYSTEM_READ_TRANSACTIONS_DEBITS_VIEW_ID,
-            SYSTEM_READ_TRANSACTIONS_CREDITS_VIEW_ID,
-            SYSTEM_READ_TRANSACTIONS_DETAIL_VIEW_ID,
-            SYSTEM_READ_ACCOUNTS_BERLIN_GROUP_VIEW_ID,
-            SYSTEM_READ_BALANCES_BERLIN_GROUP_VIEW_ID,
             SYSTEM_READ_TRANSACTIONS_BERLIN_GROUP_VIEW_ID,
             SYSTEM_INITIATE_PAYMENTS_BERLIN_GROUP_VIEW_ID
           )
           for {
             systemView <- additionalSystemViewsFromProps
             if additionalSystemViews.exists(_ == systemView)
           } {
-            Views.views.vend.getOrCreateSystemView(systemView)
+            Views.views.vend.ensureSystemViewUpToDate(systemView)
           }
         case _ => // Do nothing
       }

obp-api/src/main/scala/code/views/MapperViews.scala

@@ -717,6 +717,46 @@ object MapperViews extends Views with MdcLoggable {
   }
 
 
+  /**
+   * Create the view if it is missing, and bring an existing one's permissions back in line with
+   * `applyDefaultsForSystemView`.
+   *
+   * getOrCreateSystemView returns an existing row untouched, and applyDefaultsForSystemView runs
+   * only from unsavedSystemView (creation) and factoryResetSystemView (an admin endpoint). So a
+   * view created by an older version keeps that version's permission set for good: tightening a
+   * set in code reaches new installations and no others.
+   *
+   * Re-applied rather than compared-then-applied. resetViewPermissions already deletes before it
+   * inserts, so the write is idempotent, and at nine views once per boot the cost is not worth a
+   * second source of truth for what each view's target set is. The before/after comparison is kept
+   * for the log line only -- an operator upgrading should be able to see exactly which views moved.
+   *
+   * A view id applyDefaultsForSystemView does not know falls through its `case _`, so nothing is
+   * written and this is then just getOrCreateSystemView.
+   *
+   * Only permissions. Unlike factoryResetSystemView this leaves name, description, isPublic_ and
+   * the alias flags alone, so an operator's edits to those survive an upgrade.
+   */
+  def ensureSystemViewUpToDate(viewId: String) : Box[View] = {
+    for {
+      _ <- getOrCreateSystemView(viewId)
+      entity <- ViewDefinition.findSystemView(viewId) ?~! s"$SystemViewNotFound $viewId"
+    } yield {
+      val before = entity.allowed_actions.toSet
+      applyDefaultsForSystemView(entity, viewId)
+      val saved = entity.saveMe()
+      val after = saved.allowed_actions.toSet
+      if (after != before) {
+        logger.warn(
+          s"ensureSystemViewUpToDate: system view $viewId carried ${before.size} permission(s) " +
+          s"but the code defines ${after.size}. Brought into line. " +
+          s"Removed: ${(before -- after).toList.sorted.mkString(", ")}. " +
+          s"Added: ${(after -- before).toList.sorted.mkString(", ")}.")
+      }
+      saved
+    }
+  }
+
   def getOrCreateSystemView(viewId: String) : Box[View] = {
     getExistingSystemView(viewId) match {
       case Empty =>

obp-api/src/main/scala/code/views/Views.scala

@@ -119,6 +119,19 @@ trait Views {
    */
   def factoryResetSystemView(viewId: ViewId) : Box[View]
 
+  /**
+   * Get or create a system view AND bring its permissions into line with the code.
+   *
+   * getOrCreateSystemView returns an existing row untouched, so a view created by an older
+   * version keeps whatever permission set that version gave it -- a change to a view's can_*
+   * set in code therefore reaches new installations only. This is what boot calls instead, so
+   * the code stays the source of truth for the views it defines on every installation.
+   *
+   * Only the permission set is reconciled. Unlike factoryResetSystemView this leaves the row's
+   * name, description and flags alone, so an operator's edits to those survive.
+   */
+  def ensureSystemViewUpToDate(viewId: String) : Box[View]
+
   def getOwners(view: View): Set[User]
 
   def removeAllAccountAccess(bankId: BankId, accountId: AccountId) : Boolean

obp-api/src/test/scala/code/views/MappedViewsTest.scala

@@ -129,6 +129,89 @@ class MappedViewsTest extends ServerSetup with DefaultUsers{
       actionsOf(Constant.SYSTEM_READ_BALANCES_VIEW_ID) should equal(Set(Constant.CAN_SEE_BANK_ACCOUNT_BALANCE, Constant.CAN_QUERY_AVAILABLE_FUNDS))
     }
 
+    /**
+     * The scenario above proves the permission sets are right when a view is *created*. That is
+     * the only path it can reach: afterEach drops every ViewDefinition, so every scenario starts
+     * on an empty table and getOrCreateSystemView always takes its create branch.
+     *
+     * The upgrade path is the one that matters and was never covered. applyDefaultsForSystemView
+     * runs from unsavedSystemView (creation) and factoryResetSystemView (an admin endpoint);
+     * getOrCreateSystemView -- what boot calls -- returns an existing row untouched. So on a
+     * database where these views already exist carrying the generic set an older version gave
+     * them, tightening the sets in code changes nothing at all: "Detail" still grants nothing
+     * beyond "Basic", and Balances alone still exposes transaction and counterparty data.
+     *
+     * These build that database and then do what boot does.
+     */
+    val upgradedViews = List(
+      Constant.SYSTEM_READ_ACCOUNTS_BASIC_VIEW_ID -> Constant.SYSTEM_READ_ACCOUNTS_BASIC_VIEW_PERMISSION,
+      Constant.SYSTEM_READ_ACCOUNTS_DETAIL_VIEW_ID -> Constant.SYSTEM_READ_ACCOUNTS_DETAIL_VIEW_PERMISSION,
+      Constant.SYSTEM_READ_BALANCES_VIEW_ID -> Constant.SYSTEM_READ_BALANCES_VIEW_PERMISSION,
+      Constant.SYSTEM_READ_TRANSACTIONS_BASIC_VIEW_ID -> Constant.SYSTEM_READ_TRANSACTIONS_BASIC_VIEW_PERMISSION,
+      Constant.SYSTEM_READ_TRANSACTIONS_DEBITS_VIEW_ID -> Constant.SYSTEM_READ_TRANSACTIONS_DEBITS_VIEW_PERMISSION,
+      Constant.SYSTEM_READ_TRANSACTIONS_CREDITS_VIEW_ID -> Constant.SYSTEM_READ_TRANSACTIONS_CREDITS_VIEW_PERMISSION,
+      Constant.SYSTEM_READ_TRANSACTIONS_DETAIL_VIEW_ID -> Constant.SYSTEM_READ_TRANSACTIONS_DETAIL_VIEW_PERMISSION,
+      Constant.SYSTEM_READ_ACCOUNTS_BERLIN_GROUP_VIEW_ID -> Constant.SYSTEM_READ_ACCOUNTS_BERLIN_GROUP_VIEW_PERMISSION,
+      Constant.SYSTEM_READ_BALANCES_BERLIN_GROUP_VIEW_ID -> Constant.SYSTEM_READ_BALANCES_BERLIN_GROUP_VIEW_PERMISSION
+    )
+
+    /** A database written by the version that gave all of these one generic permission set. */
+    def seedPreUpgradeDatabase(): Unit = upgradedViews.foreach { case (viewId, _) =>
+      val view = MapperViews.getOrCreateSystemView(viewId)
+        .openOrThrowException(s"$viewId should be a known system view")
+      ViewPermission.resetViewPermissions(view, Constant.SYSTEM_VIEW_PERMISSION_COMMON)
+    }
+
+    def permissionsOf(viewId: String): Set[String] =
+      ViewDefinition.findSystemView(viewId)
+        .openOrThrowException(s"$viewId should exist by now").allowed_actions.toSet
+
+    scenario("an upgrade brings existing system views into line with the code") {
+      Given("a database whose nine UK/BG views carry the old generic permission set")
+      seedPreUpgradeDatabase()
+      permissionsOf(Constant.SYSTEM_READ_BALANCES_VIEW_ID) should
+        equal(Constant.SYSTEM_VIEW_PERMISSION_COMMON.toSet)
+
+      When("boot sets the system views up")
+      upgradedViews.foreach { case (viewId, _) => MapperViews.ensureSystemViewUpToDate(viewId) }
+
+      Then("every one of them matches what the code defines")
+      upgradedViews.foreach { case (viewId, expected) =>
+        withClue(s"$viewId: ") { permissionsOf(viewId) should equal(expected.toSet) }
+      }
+
+      And("Balances in particular no longer carries transaction or counterparty visibility")
+      val balances = permissionsOf(Constant.SYSTEM_READ_BALANCES_VIEW_ID)
+      balances.filter(_.contains("transaction")) shouldBe empty
+      balances.filter(_.contains("other_account")) shouldBe empty
+    }
+
+    scenario("reconciling twice is a no-op, not a second write") {
+      seedPreUpgradeDatabase()
+      upgradedViews.foreach { case (viewId, _) => MapperViews.ensureSystemViewUpToDate(viewId) }
+      val afterFirst = upgradedViews.map { case (viewId, _) => viewId -> permissionsOf(viewId) }.toMap
+
+      When("boot runs again, as it does on every restart")
+      upgradedViews.foreach { case (viewId, _) => MapperViews.ensureSystemViewUpToDate(viewId) }
+
+      Then("nothing has changed, and no duplicate rows have accumulated")
+      upgradedViews.foreach { case (viewId, _) =>
+        withClue(s"$viewId: ") {
+          permissionsOf(viewId) should equal(afterFirst(viewId))
+          val rows = ViewPermission.findSystemViewPermissions(ViewId(viewId))
+          rows.map(_.permission.get).distinct.size should equal(rows.size)
+        }
+      }
+    }
+
+    scenario("a view the code does not define is left alone") {
+      val owner = MapperViews.getOrCreateSystemView(Constant.SYSTEM_OWNER_VIEW_ID)
+        .openOrThrowException("owner should be a known system view")
+      val before = owner.allowed_actions.toSet
+      MapperViews.ensureSystemViewUpToDate(Constant.SYSTEM_READ_BALANCES_VIEW_ID)
+      permissionsOf(Constant.SYSTEM_OWNER_VIEW_ID) should equal(before)
+    }
+
   }