Merge pull request #203 from openlawlibrary/renatav/error-if-unauthen…

…ticated-fixes

Error if unauthenticated fixes

CHANGELOG.md

@@ -15,11 +15,16 @@ and this project adheres to [Semantic Versioning][semver].
 
 ### Changed
 
+- ​Specify a list of repositories which shouldn't contain additional commits instead of just specifying a flag ([203])
 
 ### Fixed
 
+- Raise an error if a repository which should not contain additional commits does so ([203])
+- Do not merge target commits if update as a whole will later fail ([203])
 
 
+[203]: https://github.com/openlawlibrary/taf/pull/203
+
 
 ## [0.13.4] - 01/20/2022
 

taf/tests/test_updater.py

@@ -104,7 +104,6 @@ def run_around_tests(client_dir):
     "test_name, test_repo",
     [
         ("test-updater-valid", UpdateType.OFFICIAL),
-        ("test-updater-additional-target-commit", UpdateType.OFFICIAL),
         ("test-updater-valid-with-updated-expiration-dates", UpdateType.OFFICIAL),
         ("test-updater-allow-unauthenticated-commits", UpdateType.OFFICIAL),
         ("test-updater-test-repo", UpdateType.TEST),
@@ -127,7 +126,6 @@ def test_valid_update_no_client_repo(
     "test_name, test_repo",
     [
         ("test-updater-valid", UpdateType.OFFICIAL),
-        ("test-updater-additional-target-commit", UpdateType.OFFICIAL),
         ("test-updater-allow-unauthenticated-commits", UpdateType.OFFICIAL),
         ("test-updater-multiple-branches", UpdateType.OFFICIAL),
         ("test-updater-delegated-roles", UpdateType.OFFICIAL),
@@ -146,7 +144,6 @@ def test_valid_update_no_auth_repo_one_target_repo_exists(
     "test_name, num_of_commits_to_revert",
     [
         ("test-updater-valid", 3),
-        ("test-updater-additional-target-commit", 1),
         ("test-updater-allow-unauthenticated-commits", 1),
         ("test-updater-multiple-branches", 5),
         ("test-updater-delegated-roles", 1),
@@ -203,6 +200,7 @@ def test_no_update_necessary(
     "test_name, expected_error",
     [
         ("test-updater-invalid-target-sha", TARGET1_SHA_MISMATCH),
+        ("test-updater-additional-target-commit", TARGET1_SHA_MISMATCH),
         ("test-updater-missing-target-commit", TARGET1_SHA_MISMATCH),
         ("test-updater-wrong-key", NO_WORKING_MIRRORS),
         ("test-updater-invalid-version-number", REPLAYED_METADATA),

taf/tools/repo/__init__.py

@@ -191,13 +191,13 @@ def initialize(path, library_dir, namespace, targets_rel_dir, custom, use_mirror
     @click.option("--expected-repo-type", default="either", type=click.Choice(["test", "official", "either"]),
                   help="Indicates expected authentication repository type - test or official. If type is set to either, "
                   "the updater will not check the repository's type")
-    @click.option("--error-if-unauthenticated", is_flag=True, help="Raise an error if the repository allows "
+    @click.option("--error-if-unauthenticated-repos-list", multiple=True, help="Raise an error if the repository allows "
                   "unauthentiated commits and the updater detected authenticated commits newer than local "
                   "head commit")
     @click.option("--scripts-root-dir", default=None, help="Scripts root directory, which can be used to move scripts "
                   "out of the authentication repository for testing purposes (avoid dirty index). Scripts will be expected "
                   "to be located in scripts_root_dir/repo_name directory")
-    def update(url, clients_auth_path, clients_library_dir, default_branch, from_fs, expected_repo_type, error_if_unauthenticated,
+    def update(url, clients_auth_path, clients_library_dir, default_branch, from_fs, expected_repo_type, error_if_unauthenticated_repos_list,
                scripts_root_dir):
         """
         Update and validate local authentication repository and target repositories. Remote
@@ -229,7 +229,7 @@ def update(url, clients_auth_path, clients_library_dir, default_branch, from_fs,
         """
         update_repository(url, clients_auth_path, clients_library_dir, default_branch, from_fs,
                           UpdateType(expected_repo_type),
-                          error_if_unauthenticated=error_if_unauthenticated, scripts_root_dir=scripts_root_dir)
+                          error_if_unauthenticated_repos_list=error_if_unauthenticated_repos_list, scripts_root_dir=scripts_root_dir)
 
     @repo.command()
     @click.argument("clients-auth-path")

taf/updater/updater.py

@@ -125,7 +125,7 @@ def update_repository(
     target_factory=None,
     only_validate=False,
     validate_from_commit=None,
-    error_if_unauthenticated=False,
+    error_if_unauthenticated_repos_list=None,
     conf_directory_root=None,
     config_path=None,
     out_of_band_authentication=None,
@@ -158,6 +158,8 @@ def update_repository(
     # if the repository's name is not provided, divide it in parent directory
     # and repository name, since TUF's updater expects a name
     # but set the validate_repo_name setting to False
+    if error_if_unauthenticated_repos_list is None:
+        error_if_unauthenticated_repos_list = []
     clients_auth_path = Path(clients_auth_path).resolve()
 
     if clients_library_dir is None:
@@ -183,7 +185,7 @@ def update_repository(
             target_factory,
             only_validate,
             validate_from_commit,
-            error_if_unauthenticated,
+            error_if_unauthenticated_repos_list,
             conf_directory_root,
             repos_update_data=repos_update_data,
             transient_data=transient_data,
@@ -263,7 +265,7 @@ def _update_named_repository(
     target_factory=None,
     only_validate=False,
     validate_from_commit=None,
-    error_if_unauthenticated=False,
+    error_if_unauthenticated_repos_list=None,
     conf_directory_root=None,
     visited=None,
     hosts_hierarchy_per_repo=None,
@@ -322,6 +324,8 @@ def _update_named_repository(
     """
     if visited is None:
         visited = []
+    if error_if_unauthenticated_repos_list is None:
+        error_if_unauthenticated_repos_list = []
     # if there is a recursive dependency
     if auth_repo_name in visited:
         return
@@ -345,7 +349,7 @@ def _update_named_repository(
         target_factory,
         only_validate,
         validate_from_commit,
-        error_if_unauthenticated,
+        error_if_unauthenticated_repos_list,
         conf_directory_root,
         out_of_band_authentication,
         checkout,
@@ -404,7 +408,7 @@ def _update_named_repository(
                         target_factory,
                         only_validate,
                         validate_from_commit,
-                        error_if_unauthenticated,
+                        error_if_unauthenticated_repos_list,
                         conf_directory_root,
                         visited,
                         hosts_hierarchy_per_repo,
@@ -480,7 +484,7 @@ def _update_current_repository(
     target_factory,
     only_validate,
     validate_from_commit,
-    error_if_unauthenticated,
+    error_if_unauthenticated_repos_list,
     conf_directory_root,
     out_of_band_authentication,
     checkout,
@@ -609,7 +613,7 @@ def _commits_ret(commits, existing_repo, update_successful):
             repositories_branches_and_commits,
             last_validated_commit,
             only_validate,
-            error_if_unauthenticated,
+            error_if_unauthenticated_repos_list,
             checkout,
         )
     except Exception as e:
@@ -627,7 +631,11 @@ def _commits_ret(commits, existing_repo, update_successful):
     finally:
         repository_updater.update_handler.cleanup()
 
-    if error_if_unauthenticated and len(additional_commits_per_repo):
+    unauthenticated_repo = any(
+        repo in error_if_unauthenticated_repos_list
+        for repo in additional_commits_per_repo
+    )
+    if unauthenticated_repo:
         return (
             Event.FAILED,
             users_auth_repo,
@@ -708,7 +716,7 @@ def _update_target_repositories(
     repositories_branches_and_commits,
     last_validated_commit,
     only_validate,
-    error_if_unauthenticated,
+    error_if_unauthenticated_repos_list,
     checkout,
 ):
     taf_logger.info("Validating target repositories")
@@ -720,6 +728,7 @@ def _update_target_repositories(
     additional_commits_per_repo = {}
     top_commits_of_branches_before_pull = {}
     for path, repository in repositories.items():
+        error_if_unauthenticated = path in error_if_unauthenticated_repos_list
         taf_logger.info("Validating repository {}", repository.name)
         allow_unauthenticated_for_repo = repository.custom.get(
             "allow-unauthenticated-commits", False
@@ -810,24 +819,27 @@ def _update_target_repositories(
                 # TODO is it important to undo a fetch if the repository was not cloned?
                 raise e
 
-    taf_logger.info("Successfully validated all target repositories.")
-    if not only_validate:
-        # if update is successful, merge the commits
-        for path, repository in repositories.items():
-            for branch in repositories_branches_and_commits[path]:
-                branch_commits = repositories_branches_and_commits[path][branch]
-                if not len(branch_commits):
-                    continue
-                _merge_branch_commits(
-                    repository,
-                    branch,
-                    branch_commits,
-                    allow_unauthenticated[path],
-                    additional_commits_per_repo.get(path, {}).get(branch),
-                    new_commits[path][branch],
-                    checkout,
-                )
-
+    if not len(additional_commits_per_repo) or not error_if_unauthenticated:
+        taf_logger.info("Successfully validated all target repositories.")
+        # do not merge commits if there there are
+        if not only_validate:
+            # if update is successful, merge the commits
+            for path, repository in repositories.items():
+                for branch in repositories_branches_and_commits[path]:
+                    branch_commits = repositories_branches_and_commits[path][branch]
+                    if not len(branch_commits):
+                        continue
+                    _merge_branch_commits(
+                        repository,
+                        branch,
+                        branch_commits,
+                        allow_unauthenticated[path],
+                        additional_commits_per_repo.get(path, {}).get(branch),
+                        new_commits[path][branch],
+                        checkout,
+                    )
+    else:
+        taf_logger.error("Unauthenticated commits not allowed")
     return additional_commits_per_repo, _set_target_repositories_data(
         repositories,
         repositories_branches_and_commits,
@@ -1045,13 +1057,12 @@ def _update_target_repository(
                     break
         if len(new_commits) > len(target_commits):
             additional_commits = new_commits[len(target_commits) :]
-            taf_logger.warning(
-                "Found commits {} in repository {} that are not accounted for in the authentication repo."
-                "Repository will be updated up to commit {}",
+            taf_logger.error(
+                "Found commits {} in repository {} that are not accounted for in the authentication repo. Unauthenticated commits are not allowed in this repo.",
                 additional_commits,
                 repository.name,
-                target_commits[-1],
             )
+            update_successful = False
     else:
         taf_logger.info(
             "Unauthenticated commits allowed in repository {}", repository.name