From 0d44a3e58f88d0f86da29997e0b04f263b2afbda Mon Sep 17 00:00:00 2001
From: Martin Vrachev <mvrachev@vmware.com>
Date: Tue, 19 Dec 2023 13:40:25 +0200
Subject: [PATCH 1/5] Refactor update_snapshot & _run_online_roles_bump

The goal of the "update_snapshot" refactoring is to:
1. make a better separation between the two usages of "update_snapshot"
- the first is to update snapshot and all roles that were given with the
"targets_roles" argument
- the second is to update snapshot and ALL bin target roles
In the end, I wanted to be able to call "update_snapshot" with
"bump_all" without having to pass the "targets_roles" argument.
2. fix a bug where when calling "update_snapshot" the resulting new
snapshot meta didn't contain updated information for targets.json
This resulted in a bug that when calling "_run_online_roles_bump", which
updated targets.json, we saved the new targets.json, but the new
snapshot.json didn't reference it.

The second part of this refactoring is about "_run_online_roles_bump".
Again there were two goals of this refactoring:
1. make sure we call "update_snapshot" correctly. This means when we
force an update of all target bins we shouldn't also pass the
"targets_roles" list.
2. make it clearer how we use the "force" argument:
- when "force = True" we force a bump of all bins and targets.json
- when "force = False" we bump only those bins and targets.json which
have expired.

Finally, I did some small tweaks on our tests with the idea to improve
them our asserts and checks.

Signed-off-by: Martin Vrachev <martin.vrachev@broadcom.com>
---
 repository_service_tuf_worker/repository.py   | 182 ++++++------
 .../test_repository.py                        | 259 ++++++++++++------
 2 files changed, 275 insertions(+), 166 deletions(-)

diff --git a/repository_service_tuf_worker/repository.py b/repository_service_tuf_worker/repository.py
index 2b596341..3e741baf 100644
--- a/repository_service_tuf_worker/repository.py
+++ b/repository_service_tuf_worker/repository.py
@@ -286,67 +286,76 @@ def _update_snapshot(
         Returns the new snapshot version.
 
         Args:
-            bump_all: Wheter to bump all bin target roles.
+            target_roles: List of roles to bump. If provided, 'bump_all' arg
+                will NOT be taken into account.
+            bump_all: Wheter to bump all bin target roles. If provided, then
+                'target_roles' arg is NOT taken into acount.
         """
         snapshot: Metadata[Snapshot] = self._storage_backend.get(Snapshot.type)
 
+        db_target_roles: List[targets_models.RSTUFTargetRoles] = []
         if target_roles:
-            db_target_roles: List[targets_models.RSTUFTargetRoles] = []
-            if bump_all:
-                db_target_roles = targets_crud.read_all_roles(self._db)
-                for db_role in db_target_roles:
-                    rolename = db_role.rolename
-                    bins_md: Metadata[Targets] = self._storage_backend.get(
-                        db_role.rolename
-                    )
-                    # update expiry, bump version and persist to the storage
-                    self._bump_and_persist(bins_md, BINS, persist=False)
-                    self._persist(bins_md, db_role.rolename)
+            db_target_roles = targets_crud.read_roles_joint_files(
+                self._db, target_roles
+            )
 
-                    snapshot.signed.meta[f"{rolename}.json"] = MetaFile(
-                        version=bins_md.signed.version
-                    )
-            else:
-                db_target_roles = targets_crud.read_roles_joint_files(
-                    self._db, target_roles
+            for db_role in db_target_roles:
+                rolename = db_role.rolename
+                bins_md: Metadata[Targets] = self._storage_backend.get(
+                    rolename
+                )
+                bins_md.signed.targets.clear()
+                bins_md.signed.targets = {
+                    file.path: TargetFile.from_dict(file.info, file.path)
+                    for file in db_role.target_files
+                    if file.action == targets_schema.TargetAction.ADD
+                    # Filtering the files with action 'ADD' cannot be done
+                    # in CRUD. If a target role doesn't have any target
+                    # files with an action 'ADD' (only 'REMOVE') then using
+                    # CRUD will not return the target role and it won't be
+                    # updated. An example can be when there is a role with
+                    # one target file with action "REMOVE" and the CRUD
+                    # will return None for this specific role.
+                }
+
+                # update expiry, bump version and persist to the storage
+                self._bump_and_persist(bins_md, BINS, persist=False)
+                self._persist(bins_md, rolename)
+                # update targetfile in db
+                # note: It update only if is not published see the CRUD.
+                targets_crud.update_files_to_published(
+                    self._db, [file.path for file in db_role.target_files]
                 )
 
-                for db_role in db_target_roles:
-                    rolename = db_role.rolename
-                    bins_md: Metadata[Targets] = self._storage_backend.get(
-                        rolename
-                    )
-                    bins_md.signed.targets.clear()
-                    bins_md.signed.targets = {
-                        file.path: TargetFile.from_dict(file.info, file.path)
-                        for file in db_role.target_files
-                        if file.action == targets_schema.TargetAction.ADD
-                        # Filtering the files with action 'ADD' cannot be done
-                        # in CRUD. If a target role doesn't have any target
-                        # files with an action 'ADD' (only 'REMOVE') then using
-                        # CRUD will not return the target role and it won't be
-                        # updated. An example can be when there is a role with
-                        # one target file with action "REMOVE" and the CRUD
-                        # will return None for this specific role.
-                    }
-
-                    # update expiry, bump version and persist to the storage
-                    self._bump_and_persist(bins_md, BINS, persist=False)
-                    self._persist(bins_md, rolename)
-                    # update targetfile in db
-                    # note: It update only if is not published see the CRUD.
-                    targets_crud.update_files_to_published(
-                        self._db, [file.path for file in db_role.target_files]
-                    )
+                snapshot.signed.meta[f"{rolename}.json"] = MetaFile(
+                    version=bins_md.signed.version
+                )
 
-                    snapshot.signed.meta[f"{rolename}.json"] = MetaFile(
-                        version=bins_md.signed.version
-                    )
+        elif bump_all:
+            db_target_roles = targets_crud.read_all_roles(self._db)
+            for db_role in db_target_roles:
+                rolename = db_role.rolename
+                bins_md: Metadata[Targets] = self._storage_backend.get(
+                    db_role.rolename
+                )
+                # update expiry, bump version and persist to the storage
+                self._bump_and_persist(bins_md, BINS, persist=False)
+                self._persist(bins_md, db_role.rolename)
+
+                snapshot.signed.meta[f"{rolename}.json"] = MetaFile(
+                    version=bins_md.signed.version
+                )
 
+        if len(db_target_roles) > 0:
             targets_crud.update_roles_version(
                 self._db, [int(db_role.id) for db_role in db_target_roles]
             )
 
+        targets: Metadata[Targets] = self._storage_backend.get(Targets.type)
+        snapshot.signed.meta[f"{Targets.type}.json"] = MetaFile(
+            version=targets.signed.version
+        )
+
         # update expiry, bump version and persist to the storage
         self._bump_and_persist(snapshot, Snapshot.type)
 
@@ -958,17 +967,17 @@ def _run_online_roles_bump(self, force: Optional[bool] = False) -> bool:
         signed and persisted using the configured key and storage services.
 
         Args:
-            force: force target roles bump if they don't match the hours before
-                expire (`self._hours_before_expire`)
+            force: force all target roles bump even if they have more than
+            `self._hours_before_expire` hours to expire.
         """
-        targets_roles: List[str] = []
-
         try:
-            targets = self._storage_backend.get(Targets.type)
+            targets: Metadata = self._storage_backend.get(Targets.type)
         except StorageError:
             logging.error(f"{Targets.type} not found, not bumping.")
             return False
 
+        timestamp: Metadata
+        snapshot_bump = False
         if self._settings.get_fresh("TARGETS_ONLINE_KEY") is None:
             logging.critical("No configuration found for TARGETS_ONLINE_KEY")
 
@@ -976,45 +985,54 @@ def _run_online_roles_bump(self, force: Optional[bool] = False) -> bool:
             logging.warning(
                 f"{Targets.type} don't use online key, skipping 'Targets' role"
             )
-
         else:
-            if (targets.signed.expires - datetime.now()) < timedelta(
+            if force or (targets.signed.expires - datetime.now()) < timedelta(
                 hours=self._hours_before_expire
             ):
+                logging.info("Bumped version of 'Targets' role")
                 self._bump_and_persist(targets, Targets.type)
-                targets_roles.append(Targets.type)
+                snapshot_bump = True
 
-        targets_succinct_roles = targets.signed.delegations.succinct_roles
-        for bins_name in targets_succinct_roles.get_roles():
-            bins_role: Metadata[Targets] = self._storage_backend.get(bins_name)
-
-            if (bins_role.signed.expires - datetime.now()) < timedelta(
-                hours=self._hours_before_expire or force is True
-            ):
-                targets_roles.append(bins_name)
-
-        if len(targets_roles) > 0:
-            logging.info(
-                "[scheduled targets bump] Targets and delegated Targets roles "
-                "version bumped: {targets_meta}"
-            )
+        if force:
+            # Updating all bin target roles.
             timestamp = self._update_timestamp(
-                self._update_snapshot(targets_roles, bump_all=True)
+                self._update_snapshot(bump_all=True)
             )
+            snapshot_bump = True
+            logging.info("Targets and delegated Targets roles version bumped")
+        else:
+            # Updating only those bins that have expired.
+            bin_roles: List[str] = []
+            targets_succinct_roles = targets.signed.delegations.succinct_roles
+            for bin in targets_succinct_roles.get_roles():
+                bin_role: Metadata[Targets] = self._storage_backend.get(bin)
+                if (bin_role.signed.expires - datetime.now()) < timedelta(
+                    hours=self._hours_before_expire or force is True
+                ):
+                    bin_roles.append(bin)
+
+            if len(bin_roles) > 0:
+                timestamp = self._update_timestamp(
+                    self._update_snapshot(target_roles=bin_roles)
+                )
+                snapshot_bump = True
+                bins = "".join(bin_roles)
+                logging.info(f"Bumped versions of expired bin roles: {bins}")
+            else:
+                logging.debug(
+                    "[scheduled bump] All bin roles have more than "
+                    f"{self._hours_before_expire} hour(s) to expire, "
+                    "skipping"
+                )
+
+        if snapshot_bump:
+            snapshot_v = timestamp.signed.snapshot_meta.version
             logging.info(
-                "[scheduled targets bump] Snapshot version bumped: "
-                f"{timestamp.signed.snapshot_meta.version}"
+                f"[scheduled bump] Snapshot version bumped: {snapshot_v}"
             )
+            timestamp_v = timestamp.signed.version
             logging.info(
-                "[scheduled targets bump] Timestamp version bumped: "
-                f"{timestamp.signed.version} new expire "
-                f"{timestamp.signed.expires}"
-            )
-        else:
-            logging.debug(
-                "[scheduled targets bump] All more than "
-                f"{self._hours_before_expire} hour(s) to expire, "
-                "skipping"
+                f"[scheduled bump] Timestamp version bumped: {timestamp_v}"
             )
 
         return True
diff --git a/tests/unit/tuf_repository_service_worker/test_repository.py b/tests/unit/tuf_repository_service_worker/test_repository.py
index 948c5aa9..e229d645 100644
--- a/tests/unit/tuf_repository_service_worker/test_repository.py
+++ b/tests/unit/tuf_repository_service_worker/test_repository.py
@@ -10,7 +10,14 @@
 import pytest
 from celery.exceptions import ChordError
 from celery.result import states
-from tuf.api.metadata import Metadata, Root, Snapshot, Targets, Timestamp
+from tuf.api.metadata import (
+    Metadata,
+    MetaFile,
+    Root,
+    Snapshot,
+    Targets,
+    Timestamp,
+)
 
 from repository_service_tuf_worker import Dynaconf, repository
 from repository_service_tuf_worker.models import targets_schema
@@ -151,7 +158,7 @@ def test_refresh_settings_with_sql_user_password_secrets_OSError(
             test_repo.refresh_settings()
 
         assert "No permission /run/secrets/*" in str(e)
-        assert "No permission /run/secrets/*" in caplog.record_tuples[0]
+        assert "No permission /run/secrets/*" == caplog.messages[0]
 
     def test__sign(self, test_repo):
         fake_role = pretend.stub(keyids=["keyid_1"])
@@ -338,14 +345,20 @@ def fake__bump_and_persist(md, role, **kw):
 
     def test__update_snapshot(self, test_repo):
         snapshot_version = 3
+        targets_version = 4
         mocked_snapshot = pretend.stub(
             signed=pretend.stub(
                 meta={},
                 version=snapshot_version,
             )
         )
+        mocked_targets = pretend.stub(
+            signed=pretend.stub(version=targets_version)
+        )
         test_repo._storage_backend.get = pretend.call_recorder(
-            lambda *a: mocked_snapshot
+            lambda rolename: mocked_snapshot
+            if rolename == Snapshot.type
+            else mocked_targets
         )
 
         def fake__bump_and_persist(md, role, **kw):
@@ -359,8 +372,12 @@ def fake__bump_and_persist(md, role, **kw):
 
         assert result == snapshot_version + 1
         assert mocked_snapshot.signed.version == snapshot_version + 1
+        assert mocked_snapshot.signed.meta == {
+            "targets.json": MetaFile(version=targets_version)
+        }
         assert test_repo._storage_backend.get.calls == [
-            pretend.call(repository.Roles.SNAPSHOT.value)
+            pretend.call(repository.Roles.SNAPSHOT.value),
+            pretend.call(repository.Roles.TARGETS.value),
         ]
         assert test_repo._bump_and_persist.calls == [
             pretend.call(mocked_snapshot, repository.Roles.SNAPSHOT.value)
@@ -374,6 +391,8 @@ def test__update_snapshot_specific_targets(self, test_repo, monkeypatch):
         snapshot_version = 3
         bins_a_version = 4
         bins_e_version = 4
+        targets_version = 3
+        # Test that only "bins-e" is updated. "bins-a" doesn't require update.
         mocked_snapshot = pretend.stub(
             signed=pretend.stub(
                 meta={
@@ -386,10 +405,20 @@ def test__update_snapshot_specific_targets(self, test_repo, monkeypatch):
         mocked_bins_md = pretend.stub(
             signed=pretend.stub(targets={"k": "v"}, version=bins_e_version)
         )
+        mocked_targets = pretend.stub(
+            signed=pretend.stub(version=targets_version)
+        )
+
+        def get(rolename: str):
+            if rolename == Snapshot.type:
+                return mocked_snapshot
+            elif rolename == Targets.type:
+                return mocked_targets
+            else:
+                return mocked_bins_md
+
         test_repo._storage_backend.get = pretend.call_recorder(
-            lambda rolename: mocked_snapshot
-            if rolename == "snapshot"
-            else mocked_bins_md
+            lambda rolename: get(rolename)
         )
         fake_bins_e = pretend.stub(
             rolename="bins-e",
@@ -442,6 +471,7 @@ def fake__bump_and_persist(md, role, **kw):
         assert mocked_snapshot.signed.meta == {
             "bins-a.json": bins_a_version,
             "bins-e.json": bins_a_version + 1,
+            "targets.json": targets_version,
         }
         assert mocked_bins_md.signed.targets == {"k1": "f1"}
         assert repository.targets_crud.read_roles_joint_files.calls == [
@@ -456,7 +486,8 @@ def fake__bump_and_persist(md, role, **kw):
             )
         ]
         assert repository.MetaFile.calls == [
-            pretend.call(version=mocked_bins_md.signed.version)
+            pretend.call(version=mocked_bins_md.signed.version),
+            pretend.call(version=mocked_targets.signed.version),
         ]
         assert repository.targets_crud.update_roles_version.calls == [
             pretend.call(
@@ -466,6 +497,7 @@ def fake__bump_and_persist(md, role, **kw):
         assert test_repo._storage_backend.get.calls == [
             pretend.call(repository.Roles.SNAPSHOT.value),
             pretend.call("bins-e"),
+            pretend.call(repository.Roles.TARGETS.value),
         ]
         assert test_repo._bump_and_persist.calls == [
             pretend.call(
@@ -479,6 +511,7 @@ def fake__bump_and_persist(md, role, **kw):
 
     def test__update_snapshot_bump_all(self, test_repo, monkeypatch):
         snapshot_version = 3
+        targets_version = 4
         mocked_snapshot = pretend.stub(
             signed=pretend.stub(
                 meta={"bins-e.json": 2, "bins-f.json": 6},
@@ -493,20 +526,28 @@ def test__update_snapshot_bump_all(self, test_repo, monkeypatch):
                 signed=pretend.stub(targets={"k": "v"}, version=6)
             ),
         }
+        mocked_targets = pretend.stub(
+            signed=pretend.stub(version=targets_version)
+        )
+
+        def get(rolename: str):
+            if rolename == Snapshot.type:
+                return mocked_snapshot
+            elif rolename == Targets.type:
+                return mocked_targets
+            else:
+                return mocked_bins[rolename]
+
         test_repo._storage_backend.get = pretend.call_recorder(
-            lambda rolename: mocked_snapshot
-            if rolename == "snapshot"
-            else mocked_bins[rolename]
+            lambda rolename: get(rolename)
         )
         fake_bins = [
             pretend.stub(rolename="bins-e", id=3),
-            pretend.stub(
-                rolename="bins-f",
-                id=4,
-            ),
+            pretend.stub(rolename="bins-f", id=4),
         ]
         fake_read_all_roles = pretend.call_recorder(lambda *a: fake_bins)
         test_repo._db = pretend.stub()
+        repository.MetaFile = pretend.call_recorder(lambda **kw: kw["version"])
         monkeypatch.setattr(
             repository.targets_crud,
             "read_all_roles",
@@ -526,22 +567,27 @@ def fake__bump_and_persist(md, role, **kw):
             "update_roles_version",
             fake_update_roles_version,
         )
-        targets = ["bins-e", "bins-f"]
-        result = test_repo._update_snapshot(targets, bump_all=True)
+        result = test_repo._update_snapshot(bump_all=True)
 
         assert result == snapshot_version + 1
         assert mocked_snapshot.signed.version == snapshot_version + 1
-        for meta_key, meta_value in mocked_snapshot.signed.meta.items():
-            assert (
-                meta_value.to_dict()["version"]
-                == mocked_bins[meta_key.split(".json")[0]].signed.version
-            )
+        assert mocked_snapshot.signed.meta == {
+            "bins-e.json": mocked_bins["bins-e"].signed.version,
+            "bins-f.json": mocked_bins["bins-f"].signed.version,
+            "targets.json": mocked_targets.signed.version,
+        }
         assert fake_read_all_roles.calls == [pretend.call(test_repo._db)]
         assert test_repo._bump_and_persist.calls == [
             pretend.call(mocked_bins["bins-e"], "bins", persist=False),
             pretend.call(mocked_bins["bins-f"], "bins", persist=False),
             pretend.call(mocked_snapshot, "snapshot"),
         ]
+        assert test_repo._storage_backend.get.calls == [
+            pretend.call(Snapshot.type),
+            pretend.call("bins-e"),
+            pretend.call("bins-f"),
+            pretend.call(Targets.type),
+        ]
         assert test_repo._persist.calls == [
             pretend.call(mocked_bins["bins-e"], "bins-e"),
             pretend.call(mocked_bins["bins-f"], "bins-f"),
@@ -549,6 +595,11 @@ def fake__bump_and_persist(md, role, **kw):
         assert fake_update_roles_version.calls == [
             pretend.call(test_repo._db, [3, 4])
         ]
+        assert repository.MetaFile.calls == [
+            pretend.call(version=mocked_bins["bins-e"].signed.version),
+            pretend.call(version=mocked_bins["bins-f"].signed.version),
+            pretend.call(version=mocked_targets.signed.version),
+        ]
 
     def test__get_path_succinct_role(self, test_repo):
         fake_targets = pretend.stub(
@@ -573,7 +624,7 @@ def test__get_path_succinct_role(self, test_repo):
             == [pretend.call("v0.0.1/test_path.tar.gz")]
         )
         assert test_repo._storage_backend.get.calls == [
-            pretend.call("targets")
+            pretend.call(Targets.type)
         ]
 
     def test__update_task(self, test_repo, mocked_datetime):
@@ -2044,9 +2095,10 @@ def test_remove_targets_empty_targets(self, test_repo, mocked_datetime):
             },
         }
 
-    def test__run_online_roles_bump(
-        self, monkeypatch, test_repo, mocked_datetime
+    def test__run_online_roles_bump_bump_expired(
+        self, monkeypatch, test_repo, mocked_datetime, caplog
     ):
+        caplog.set_level(repository.logging.INFO)
         fake_targets = pretend.stub(
             signed=pretend.stub(
                 delegations=pretend.stub(
@@ -2065,31 +2117,25 @@ def test__run_online_roles_bump(
             )
         )
 
-        def mocked_get(role):
-            if role == "targets":
-                return fake_targets
-            else:
-                return fake_bins
-
         test_repo._storage_backend.get = pretend.call_recorder(
-            lambda r: mocked_get(r)
+            lambda rolename: fake_targets
+            if rolename == Targets.type
+            else fake_bins
         )
         fake_settings = pretend.stub(
-            get_fresh=pretend.call_recorder(lambda *a: True)
+            get_fresh=pretend.call_recorder(lambda a: True)
         )
         monkeypatch.setattr(
             repository,
             "get_repository_settings",
             lambda *a, **kw: fake_settings,
         )
-        test_repo._bump_and_persist = pretend.call_recorder(
-            lambda *a, **kw: None
-        )
+        test_repo._bump_and_persist = pretend.call_recorder(lambda *a: None)
         test_repo._update_snapshot = pretend.call_recorder(
-            lambda *a, **kw: "fake_snapshot"
+            lambda **kw: "fake_snapshot"
         )
         test_repo._update_timestamp = pretend.call_recorder(
-            lambda *a: pretend.stub(
+            lambda a: pretend.stub(
                 signed=pretend.stub(
                     snapshot_meta=pretend.stub(version=79),
                     version=87,
@@ -2100,20 +2146,79 @@ def mocked_get(role):
         result = test_repo._run_online_roles_bump()
         assert result is True
         assert test_repo._storage_backend.get.calls == [
-            pretend.call("targets"),
+            pretend.call(Targets.type),
             pretend.call("bin-a"),
         ]
         assert test_repo._bump_and_persist.calls == [
             pretend.call(fake_targets, Targets.type),
         ]
         assert test_repo._update_snapshot.calls == [
-            pretend.call(["targets", "bin-a"], bump_all=True)
+            pretend.call(target_roles=["bin-a"])
         ]
         assert test_repo._update_timestamp.calls == [
             pretend.call("fake_snapshot")
         ]
+        assert "Bumped version of 'Targets' role" == caplog.messages[0]
+        msg_2 = "Bumped versions of expired bin roles: bin-a"
+        assert msg_2 == caplog.messages[1]
+        assert "Snapshot version bumped: 79" in caplog.messages[2]
+        assert "Timestamp version bumped: 87" in caplog.messages[3]
 
-    def test__run_online_roles_bump_target_no_online_keys(
+    def test__run_online_roles_bump_force(
+        self, monkeypatch, test_repo, caplog
+    ):
+        caplog.set_level(repository.logging.INFO)
+        fake_targets = pretend.stub(
+            signed=pretend.stub(
+                version=1,
+            )
+        )
+
+        test_repo._storage_backend.get = pretend.call_recorder(
+            lambda a: fake_targets
+        )
+        fake_settings = pretend.stub(
+            get_fresh=pretend.call_recorder(lambda a: True)
+        )
+        monkeypatch.setattr(
+            repository,
+            "get_repository_settings",
+            lambda *a, **kw: fake_settings,
+        )
+        test_repo._bump_and_persist = pretend.call_recorder(lambda *a: None)
+        test_repo._update_snapshot = pretend.call_recorder(
+            lambda **kw: "fake_snapshot"
+        )
+        test_repo._update_timestamp = pretend.call_recorder(
+            lambda *a: pretend.stub(
+                signed=pretend.stub(
+                    snapshot_meta=pretend.stub(version=79),
+                    version=87,
+                    expires=datetime.datetime(2028, 6, 16, 9, 5, 1),
+                )
+            )
+        )
+        result = test_repo._run_online_roles_bump(force=True)
+        assert result is True
+        assert test_repo._storage_backend.get.calls == [
+            pretend.call(Targets.type),
+        ]
+        assert test_repo._bump_and_persist.calls == [
+            pretend.call(fake_targets, Targets.type),
+        ]
+        assert test_repo._update_snapshot.calls == [
+            pretend.call(bump_all=True)
+        ]
+        assert test_repo._update_timestamp.calls == [
+            pretend.call("fake_snapshot")
+        ]
+        assert "Bumped version of 'Targets' role" == caplog.messages[0]
+        msg_2 = "Targets and delegated Targets roles version bumped"
+        assert msg_2 == caplog.messages[1]
+        assert "Snapshot version bumped: 79" in caplog.messages[2]
+        assert "Timestamp version bumped: 87" in caplog.messages[3]
+
+    def test__run_online_roles_bump_target_targets_online_key_config_false(
         self, monkeypatch, caplog, test_repo, mocked_datetime
     ):
         caplog.set_level(repository.logging.WARNING)
@@ -2124,25 +2229,18 @@ def test__run_online_roles_bump_target_no_online_keys(
                         get_roles=pretend.call_recorder(lambda *a: ["bin-a"])
                     )
                 ),
-                expires=mocked_datetime.now(),
-                version=1,
             )
         )
-
         fake_bins = pretend.stub(
             signed=pretend.stub(
                 targets={}, version=6, expires=mocked_datetime.now()
             )
         )
 
-        def mocked_get(role):
-            if role == "targets":
-                return fake_targets
-            else:
-                return fake_bins
-
         test_repo._storage_backend.get = pretend.call_recorder(
-            lambda r: mocked_get(r)
+            lambda rolename: fake_targets
+            if rolename == Targets.type
+            else fake_bins
         )
         fake_settings = pretend.stub(
             get_fresh=pretend.call_recorder(lambda *a: False)
@@ -2153,7 +2251,7 @@ def mocked_get(role):
             lambda *a, **kw: fake_settings,
         )
         test_repo._update_snapshot = pretend.call_recorder(
-            lambda *a, **kw: "fake_snapshot"
+            lambda **kw: "fake_snapshot"
         )
         test_repo._update_timestamp = pretend.call_recorder(
             lambda *a: pretend.stub(
@@ -2165,17 +2263,15 @@ def mocked_get(role):
             )
         )
         result = test_repo._run_online_roles_bump()
-        assert (
-            "targets don't use online key, skipping 'Targets' role"
-            in caplog.record_tuples[0]
-        )
+        msg = "targets don't use online key, skipping 'Targets' role"
+        assert msg == caplog.messages[0]
         assert result is True
         assert test_repo._storage_backend.get.calls == [
-            pretend.call("targets"),
+            pretend.call(Targets.type),
             pretend.call("bin-a"),
         ]
         assert test_repo._update_snapshot.calls == [
-            pretend.call(["bin-a"], bump_all=True)
+            pretend.call(target_roles=["bin-a"])
         ]
         assert test_repo._update_timestamp.calls == [
             pretend.call("fake_snapshot")
@@ -2192,8 +2288,6 @@ def test__run_online_roles_bump_warning_missing_config(
                         get_roles=pretend.call_recorder(lambda *a: ["bin-a"])
                     )
                 ),
-                expires=mocked_datetime.now(),
-                version=1,
             )
         )
 
@@ -2203,14 +2297,10 @@ def test__run_online_roles_bump_warning_missing_config(
             )
         )
 
-        def mocked_get(role):
-            if role == "targets":
-                return fake_targets
-            else:
-                return fake_bins
-
         test_repo._storage_backend.get = pretend.call_recorder(
-            lambda r: mocked_get(r)
+            lambda rolename: fake_targets
+            if rolename == Targets.type
+            else fake_bins
         )
         test_repo._settings.get_fresh = pretend.call_recorder(lambda *a: None)
         test_repo._update_snapshot = pretend.call_recorder(
@@ -2226,23 +2316,23 @@ def mocked_get(role):
             )
         )
         result = test_repo._run_online_roles_bump()
-        assert (
-            "No configuration found for TARGETS_ONLINE_KEY"
-            in caplog.record_tuples[0]
-        )
+        msg = "No configuration found for TARGETS_ONLINE_KEY"
+        assert msg == caplog.messages[0]
         assert result is True
         assert test_repo._storage_backend.get.calls == [
-            pretend.call("targets"),
+            pretend.call(Targets.type),
             pretend.call("bin-a"),
         ]
         assert test_repo._update_snapshot.calls == [
-            pretend.call(["bin-a"], bump_all=True)
+            pretend.call(target_roles=["bin-a"])
         ]
         assert test_repo._update_timestamp.calls == [
             pretend.call("fake_snapshot")
         ]
 
-    def test__run_online_roles_bump_no_changes(self, test_repo):
+    def test__run_online_roles_bump_no_changes(self, test_repo, caplog):
+        caplog.set_level(repository.logging.DEBUG)
+        fake_time = datetime.datetime(2054, 6, 16, 8, 5, 1)
         fake_targets = pretend.stub(
             signed=pretend.stub(
                 delegations=pretend.stub(
@@ -2250,32 +2340,33 @@ def test__run_online_roles_bump_no_changes(self, test_repo):
                         get_roles=pretend.call_recorder(lambda *a: ["bin-a"])
                     )
                 ),
-                expires=datetime.datetime(2054, 6, 16, 8, 5, 1),
+                expires=fake_time,
                 version=1,
             )
         )
 
-        fake_time = datetime.datetime(2054, 6, 16, 8, 5, 1)
         fake_bins = pretend.stub(
             signed=pretend.stub(targets={}, version=6, expires=fake_time)
         )
 
-        def mocked_get(role):
-            if role == "targets":
-                return fake_targets
-            else:
-                return fake_bins
-
         test_repo._storage_backend.get = pretend.call_recorder(
-            lambda r: mocked_get(r)
+            lambda rolename: fake_targets
+            if rolename == Targets.type
+            else fake_bins
         )
 
         result = test_repo._run_online_roles_bump()
         assert result is True
         assert test_repo._storage_backend.get.calls == [
-            pretend.call("targets"),
+            pretend.call(Targets.type),
             pretend.call("bin-a"),
         ]
+        msg_1 = "No configuration found for TARGETS_ONLINE_KEY"
+        assert msg_1 == caplog.messages[0]
+        msg_2 = "All bin roles have more than 1 hour(s) to expire, skipping"
+        assert msg_2 in caplog.messages[1]
+        assert "Snapshot version bumped:" not in caplog.messages
+        assert "Timestamp version bumped:" not in caplog.messages
 
     def test__run_online_roles_bump_StorageError(self, test_repo):
         test_repo._storage_backend.get = pretend.raiser(

From aafeb7eb99459048521207a57aedd62ecda42482 Mon Sep 17 00:00:00 2001
From: Martin Vrachev <martin.vrachev@broadcom.com>
Date: Wed, 20 Dec 2023 16:56:10 +0200
Subject: [PATCH 2/5] Remove "force" check from a specific use case

In this specific "if" check we don't need to check the value of the
"force" argument as that case is already handled from a previous
check where if "force" is True we call update_snapshot with "bump_all".

Signed-off-by: Martin Vrachev <martin.vrachev@broadcom.com>
---
 repository_service_tuf_worker/repository.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/repository_service_tuf_worker/repository.py b/repository_service_tuf_worker/repository.py
index 3e741baf..fc46f764 100644
--- a/repository_service_tuf_worker/repository.py
+++ b/repository_service_tuf_worker/repository.py
@@ -1007,7 +1007,7 @@ def _run_online_roles_bump(self, force: Optional[bool] = False) -> bool:
             for bin in targets_succinct_roles.get_roles():
                 bin_role: Metadata[Targets] = self._storage_backend.get(bin)
                 if (bin_role.signed.expires - datetime.now()) < timedelta(
-                    hours=self._hours_before_expire or force is True
+                    hours=self._hours_before_expire
                 ):
                     bin_roles.append(bin)
 

From 585a951f2cea523942512fe8751a7c2c709f7f73 Mon Sep 17 00:00:00 2001
From: Martin Vrachev <martin.vrachev@broadcom.com>
Date: Wed, 20 Dec 2023 17:36:34 +0200
Subject: [PATCH 3/5] Move "force" check outside timedelta

Currently, if the check if snapshot expiration is below a certain
timedelta and "force" are True we wouldn't taken into account the
value of force.
With this fix, this is changed.

Signed-off-by: Martin Vrachev <martin.vrachev@broadcom.com>
---
 repository_service_tuf_worker/repository.py   |  4 +-
 .../test_repository.py                        | 50 ++++++++++++++++++-
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/repository_service_tuf_worker/repository.py b/repository_service_tuf_worker/repository.py
index fc46f764..850b3354 100644
--- a/repository_service_tuf_worker/repository.py
+++ b/repository_service_tuf_worker/repository.py
@@ -1059,8 +1059,8 @@ def bump_snapshot(self, force: Optional[bool] = False) -> bool:
             return False
 
         if (snapshot.signed.expires - datetime.now()) < timedelta(
-            hours=self._hours_before_expire or force is True
-        ):
+            hours=self._hours_before_expire
+        ) or force:
             timestamp = self._update_timestamp(self._update_snapshot())
             logging.info(
                 "[scheduled snapshot bump] Snapshot version bumped: "
diff --git a/tests/unit/tuf_repository_service_worker/test_repository.py b/tests/unit/tuf_repository_service_worker/test_repository.py
index e229d645..ab61af72 100644
--- a/tests/unit/tuf_repository_service_worker/test_repository.py
+++ b/tests/unit/tuf_repository_service_worker/test_repository.py
@@ -2095,7 +2095,7 @@ def test_remove_targets_empty_targets(self, test_repo, mocked_datetime):
             },
         }
 
-    def test__run_online_roles_bump_bump_expired(
+    def test__run_online_roles_bump_only_expired(
         self, monkeypatch, test_repo, mocked_datetime, caplog
     ):
         caplog.set_level(repository.logging.INFO)
@@ -2428,6 +2428,54 @@ def test_bump_snapshot_unexpired(self, test_repo):
             pretend.call("snapshot")
         ]
 
+    def test_bump_snapshot_check_force_is_acknowledged(
+        self, test_repo, caplog
+    ):
+        # Reproduce a bug where we checked if we need to update snapshot with:
+        # if (snapshot.signed.expires - datetime.now()) < timedelta(
+        #    hours=self._hours_before_expire or True
+        # )
+        # The problem is that `force` is used inside timedelta function call
+        # which could potentially distort the end result.
+
+        # In order to reproduce it we need to have such a high snapshot
+        # expiration date, that this timedelta check cannot be true, but
+        # because we pass "force=True" we expect that snapshot must be updated.
+        # The current situation as described in the previous comment is that
+        # in this case snapshot won't be updated.
+        caplog.set_level(repository.logging.DEBUG)
+        fake_snapshot = pretend.stub(
+            signed=pretend.stub(
+                expires=datetime.datetime(2080, 6, 16, 9, 5, 1),
+                version=87,
+            )
+        )
+        test_repo._storage_backend.get = pretend.call_recorder(
+            lambda *a: fake_snapshot
+        )
+        test_repo._update_snapshot = pretend.call_recorder(
+            lambda *a: "fake_snapshot"
+        )
+        test_repo._update_timestamp = pretend.call_recorder(
+            lambda *a: pretend.stub(
+                signed=pretend.stub(
+                    snapshot_meta=pretend.stub(version=79),
+                    version=87,
+                    expires=datetime.datetime(2028, 6, 16, 9, 5, 1),
+                )
+            )
+        )
+
+        result = test_repo.bump_snapshot(force=True)
+        assert result is True
+        assert test_repo._storage_backend.get.calls == [
+            pretend.call(Snapshot.type)
+        ]
+        assert test_repo._update_snapshot.calls == [pretend.call()]
+        assert test_repo._update_timestamp.calls == [
+            pretend.call("fake_snapshot")
+        ]
+
     def test_bump_snapshot_not_found(self, test_repo):
         test_repo._storage_backend.get = pretend.raiser(
             repository.StorageError

From 558ac3b60d105697f9696f8f2bf65877fcc53190 Mon Sep 17 00:00:00 2001
From: Martin Vrachev <martin.vrachev@broadcom.com>
Date: Fri, 22 Dec 2023 15:52:23 +0200
Subject: [PATCH 4/5] Add some logging

Signed-off-by: Martin Vrachev <martin.vrachev@broadcom.com>
---
 repository_service_tuf_worker/repository.py                | 7 ++++++-
 .../unit/tuf_repository_service_worker/test_repository.py  | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/repository_service_tuf_worker/repository.py b/repository_service_tuf_worker/repository.py
index 850b3354..c4212739 100644
--- a/repository_service_tuf_worker/repository.py
+++ b/repository_service_tuf_worker/repository.py
@@ -331,6 +331,9 @@ def _update_snapshot(
                     version=bins_md.signed.version
                 )
 
+            bins = "".join(target_roles)
+            logging.info(f"Bumped all expired target 'bin' roles: {bins}")
+
         elif bump_all:
             db_target_roles = targets_crud.read_all_roles(self._db)
             for db_role in db_target_roles:
@@ -345,6 +348,7 @@ def _update_snapshot(
                 snapshot.signed.meta[f"{rolename}.json"] = MetaFile(
                     version=bins_md.signed.version
                 )
+            logging.info("Bumped all target 'bin' roles")
 
         if len(db_target_roles) > 0:
             targets_crud.update_roles_version(
@@ -358,6 +362,7 @@ def _update_snapshot(
 
         # update expiry, bump version and persist to the storage
         self._bump_and_persist(snapshot, Snapshot.type)
+        logging.info("Bumped version of 'Snapshot' role")
 
         return snapshot.signed.version
 
@@ -1260,7 +1265,7 @@ def metadata_update(
                 False,
                 {
                     "message": "Metadata Update Failed",
-                    "error": "Metadata Update requires a complete bootstrap",
+                    "error": "Metadata Update requires a completed bootstrap",
                 },
             )
 
diff --git a/tests/unit/tuf_repository_service_worker/test_repository.py b/tests/unit/tuf_repository_service_worker/test_repository.py
index ab61af72..468aa1d4 100644
--- a/tests/unit/tuf_repository_service_worker/test_repository.py
+++ b/tests/unit/tuf_repository_service_worker/test_repository.py
@@ -2984,7 +2984,7 @@ def test_metadata_update_no_bootstrap(
             "last_update": mocked_datetime.now(),
             "details": {
                 "message": "Metadata Update Failed",
-                "error": "Metadata Update requires a complete bootstrap",
+                "error": "Metadata Update requires a completed bootstrap",
             },
         }
         assert test_repo._settings.get_fresh.calls == [

From 8aede98b939efb7a7a28b95844bbc60fe349f540 Mon Sep 17 00:00:00 2001
From: Martin Vrachev <martin.vrachev@broadcom.com>
Date: Fri, 22 Dec 2023 16:01:18 +0200
Subject: [PATCH 5/5] Remove StorageError exception handling

We decided that it's best to not handle a StorageError exception
and instead allow the test to fail. Fast API will fail if there is an
exception from the worker and given that the StorageError exception
has enough details there is no need for us to handle it.

Additionally, this allowed us to remove the boolean return value
from _run_online_roles_bump and bump_snapshot.

Signed-off-by: Martin Vrachev <martin.vrachev@broadcom.com>
---
 repository_service_tuf_worker/repository.py   | 27 +++----------
 .../test_repository.py                        | 40 +++++++------------
 2 files changed, 20 insertions(+), 47 deletions(-)

diff --git a/repository_service_tuf_worker/repository.py b/repository_service_tuf_worker/repository.py
index c4212739..21846c4b 100644
--- a/repository_service_tuf_worker/repository.py
+++ b/repository_service_tuf_worker/repository.py
@@ -17,10 +17,7 @@
 from celery.exceptions import ChordError
 from celery.result import AsyncResult, states
 from dynaconf.loaders import redis_loader
-from securesystemslib.exceptions import (  # type: ignore
-    StorageError,
-    UnverifiedSignatureError,
-)
+from securesystemslib.exceptions import UnverifiedSignatureError
 from securesystemslib.signer import Signature, SSlibKey
 from tuf.api.exceptions import (
     BadVersionNumberError,
@@ -960,7 +957,7 @@ def remove_targets(
             },
         )
 
-    def _run_online_roles_bump(self, force: Optional[bool] = False) -> bool:
+    def _run_online_roles_bump(self, force: Optional[bool] = False):
         """
         Bumps version and expiration date of all online roles (`Targets`,
         `Succinct Delegated` targets roles, `Timestamp` and `Snapshot`).
@@ -975,12 +972,7 @@ def _run_online_roles_bump(self, force: Optional[bool] = False) -> bool:
             force: force all target roles bump even if they have more than
             `self._hours_before_expire` hours to expire.
         """
-        try:
-            targets: Metadata = self._storage_backend.get(Targets.type)
-        except StorageError:
-            logging.error(f"{Targets.type} not found, not bumping.")
-            return False
-
+        targets: Metadata = self._storage_backend.get(Targets.type)
         timestamp: Metadata
         snapshot_bump = False
         if self._settings.get_fresh("TARGETS_ONLINE_KEY") is None:
@@ -1040,9 +1032,7 @@ def _run_online_roles_bump(self, force: Optional[bool] = False) -> bool:
                 f"[scheduled bump] Timestamp version bumped: {timestamp_v}"
             )
 
-        return True
-
-    def bump_snapshot(self, force: Optional[bool] = False) -> bool:
+    def bump_snapshot(self, force: Optional[bool] = False):
         """
         Bumps version and expiration date of TUF 'snapshot' role metadata.
 
@@ -1057,12 +1047,7 @@ def bump_snapshot(self, force: Optional[bool] = False) -> bool:
                 expire (`self._hours_before_expire`)
         """
 
-        try:
-            snapshot = self._storage_backend.get(Snapshot.type)
-        except StorageError:
-            logging.error(f"{Snapshot.type} not found, not bumping.")
-            return False
-
+        snapshot = self._storage_backend.get(Snapshot.type)
         if (snapshot.signed.expires - datetime.now()) < timedelta(
             hours=self._hours_before_expire
         ) or force:
@@ -1084,8 +1069,6 @@ def bump_snapshot(self, force: Optional[bool] = False) -> bool:
                 f"{self._hours_before_expire} hour, skipping"
             )
 
-        return True
-
     def bump_online_roles(self, force: Optional[bool] = False) -> bool:
         """
         Bump online roles (Snapshot, Timestamp, Targets and BINS).
diff --git a/tests/unit/tuf_repository_service_worker/test_repository.py b/tests/unit/tuf_repository_service_worker/test_repository.py
index 468aa1d4..c8d49af6 100644
--- a/tests/unit/tuf_repository_service_worker/test_repository.py
+++ b/tests/unit/tuf_repository_service_worker/test_repository.py
@@ -10,6 +10,7 @@
 import pytest
 from celery.exceptions import ChordError
 from celery.result import states
+from securesystemslib.exceptions import StorageError
 from tuf.api.metadata import (
     Metadata,
     MetaFile,
@@ -2143,8 +2144,7 @@ def test__run_online_roles_bump_only_expired(
                 )
             )
         )
-        result = test_repo._run_online_roles_bump()
-        assert result is True
+        test_repo._run_online_roles_bump()
         assert test_repo._storage_backend.get.calls == [
             pretend.call(Targets.type),
             pretend.call("bin-a"),
@@ -2198,8 +2198,7 @@ def test__run_online_roles_bump_force(
                 )
             )
         )
-        result = test_repo._run_online_roles_bump(force=True)
-        assert result is True
+        test_repo._run_online_roles_bump(force=True)
         assert test_repo._storage_backend.get.calls == [
             pretend.call(Targets.type),
         ]
@@ -2262,10 +2261,9 @@ def test__run_online_roles_bump_target_targets_online_key_config_false(
                 )
             )
         )
-        result = test_repo._run_online_roles_bump()
+        test_repo._run_online_roles_bump()
         msg = "targets don't use online key, skipping 'Targets' role"
         assert msg == caplog.messages[0]
-        assert result is True
         assert test_repo._storage_backend.get.calls == [
             pretend.call(Targets.type),
             pretend.call("bin-a"),
@@ -2315,10 +2313,9 @@ def test__run_online_roles_bump_warning_missing_config(
                 )
             )
         )
-        result = test_repo._run_online_roles_bump()
+        test_repo._run_online_roles_bump()
         msg = "No configuration found for TARGETS_ONLINE_KEY"
         assert msg == caplog.messages[0]
-        assert result is True
         assert test_repo._storage_backend.get.calls == [
             pretend.call(Targets.type),
             pretend.call("bin-a"),
@@ -2355,8 +2352,7 @@ def test__run_online_roles_bump_no_changes(self, test_repo, caplog):
             else fake_bins
         )
 
-        result = test_repo._run_online_roles_bump()
-        assert result is True
+        test_repo._run_online_roles_bump()
         assert test_repo._storage_backend.get.calls == [
             pretend.call(Targets.type),
             pretend.call("bin-a"),
@@ -2370,11 +2366,11 @@ def test__run_online_roles_bump_no_changes(self, test_repo, caplog):
 
     def test__run_online_roles_bump_StorageError(self, test_repo):
         test_repo._storage_backend.get = pretend.raiser(
-            repository.StorageError("Overwrite it")
+            StorageError("Overwrite it")
         )
 
-        result = test_repo._run_online_roles_bump()
-        assert result is False
+        with pytest.raises(StorageError):
+            test_repo._run_online_roles_bump()
 
     def test_bump_snapshot(self, test_repo, mocked_datetime):
         fake_snapshot = pretend.stub(
@@ -2400,8 +2396,7 @@ def test_bump_snapshot(self, test_repo, mocked_datetime):
             )
         )
 
-        result = test_repo.bump_snapshot()
-        assert result is True
+        test_repo.bump_snapshot()
         assert test_repo._storage_backend.get.calls == [
             pretend.call("snapshot")
         ]
@@ -2422,8 +2417,7 @@ def test_bump_snapshot_unexpired(self, test_repo):
             lambda *a: fake_snapshot
         )
 
-        result = test_repo.bump_snapshot()
-        assert result is True
+        test_repo.bump_snapshot()
         assert test_repo._storage_backend.get.calls == [
             pretend.call("snapshot")
         ]
@@ -2466,8 +2460,7 @@ def test_bump_snapshot_check_force_is_acknowledged(
             )
         )
 
-        result = test_repo.bump_snapshot(force=True)
-        assert result is True
+        test_repo.bump_snapshot(force=True)
         assert test_repo._storage_backend.get.calls == [
             pretend.call(Snapshot.type)
         ]
@@ -2477,12 +2470,9 @@ def test_bump_snapshot_check_force_is_acknowledged(
         ]
 
     def test_bump_snapshot_not_found(self, test_repo):
-        test_repo._storage_backend.get = pretend.raiser(
-            repository.StorageError
-        )
-
-        result = test_repo.bump_snapshot()
-        assert result is False
+        test_repo._storage_backend.get = pretend.raiser(StorageError)
+        with pytest.raises(StorageError):
+            test_repo.bump_snapshot()
 
     def test_bump_online_roles(self, monkeypatch, test_repo):
         @contextmanager