#2195 dist-git: don't setgid(apache) while importing ("uploading")
Merged 3 months ago by praiskup. Opened 3 months ago by praiskup.
copr/ praiskup/copr dont-setgid  into  main

@@ -4,7 +4,6 @@ 

  import os

  import shutil

  import types

- import grp

  import subprocess

  import tempfile

  import munch
@@ -38,11 +37,6 @@ 

          destination = os.path.join(opts.lookaside_location, reponame,

                                     filename, filehash, filename)

  

-         # hack to allow "uploading" into lookaside

-         current_gid = os.getgid()

-         apache_gid = grp.getgrnam("apache").gr_gid

-         os.setgid(apache_gid)

- 

          if not os.path.isdir(os.path.dirname(destination)):

              try:

                  os.makedirs(os.path.dirname(destination))
@@ -52,8 +46,6 @@ 

          if not os.path.exists(destination):

              shutil.copyfile(abs_filename, destination)

  

-         os.setgid(current_gid)

- 

      return my_upload

  

  

@@ -30,11 +30,6 @@ 

  

  

  @pytest.yield_fixture

- def mc_grp_getgrnam():

-     with mock.patch("grp.getgrnam") as handle:

-         yield handle

- 

- @pytest.yield_fixture

  def mc_chroot():

      with mock.patch("os.chroot") as handle:

          yield handle
@@ -175,7 +170,7 @@ 

  

  

  @pytest.fixture

- def initial_commit_everywhere(request, branches, mc_group, mc_chroot, opts_basic, mc_setup_git_repo, mc_grp_getgrnam):

+ def initial_commit_everywhere(request, branches, mc_group, mc_chroot, opts_basic, mc_setup_git_repo):

      origin, all_branches, _, _ = branches

  

      # Commit first version and compare remote side with local side.
@@ -196,7 +191,7 @@ 

      def setup_method(self, method):

          srpm_cache = {}

  

-     def test_merged_everything(self, initial_commit_everywhere, mc_setup_git_repo, mc_grp_getgrnam):

+     def test_merged_everything(self, initial_commit_everywhere, mc_setup_git_repo):

          branches, opts, v1_hash = initial_commit_everywhere

          origin, all_branches, middle_branches, border_branches = branches

  
@@ -215,7 +210,7 @@ 

          assert v3_hash != v1_hash

          assert v3_hash != v2_hash

  

-     def test_diverge_middle_branches(self, initial_commit_everywhere, mc_setup_git_repo, mc_grp_getgrnam):

+     def test_diverge_middle_branches(self, initial_commit_everywhere, mc_setup_git_repo):

          branches, opts, v1_hash = initial_commit_everywhere

          origin, all_branches, middle_branches, border_branches = branches

  
@@ -237,7 +232,7 @@ 

          assert v3_hash_a != v1_hash

          assert v3_hash_b != v1_hash

  

-     def test_no_op_1(self, initial_commit_everywhere, mc_setup_git_repo, mc_grp_getgrnam):

+     def test_no_op_1(self, initial_commit_everywhere, mc_setup_git_repo):

          branches, opts, v1_hash = initial_commit_everywhere

          origin, all_branches, middle_branches, border_branches = branches

  

@@ -28,12 +28,6 @@ 

  

  

  @pytest.yield_fixture

- def mc_grp_getgrnam():

-     with mock.patch("{}.grp.getgrnam".format(MODULE_REF)) as handle:

-         yield handle

- 

- 

- @pytest.yield_fixture

  def mc_subprocess_check_output():

      with mock.patch("{}.subprocess.check_output".format(MODULE_REF)) as handle:

          yield handle
@@ -77,7 +71,7 @@ 

  

  class TestPackageImport(Base):

  

-     def test_my_upload(self, mc_os_setgid, mc_grp_getgrnam):

+     def test_my_upload(self, mc_os_setgid):

           filename = "source"

           source_path = os.path.join(self.tmp_dir_name, filename)

           with open(source_path, "w") as handle:

I'm totally familiar with the previous practice (I was fixing related
permissions problems while deploying Copr instances before) but it seems
entirely redundant.

First, the directories have 'setgid' bit anyway, so new files are
owned by apache group anyway and thus swtiching EGID shouldn't be
needed? And second, Apache (cgit&others) processes should be OK with
just plain read-only access to those files (while "others" have the r-x
permissions there).

This fix is needed to allow running DistGit importer in OpenShift, where
we have an artificial UID and root (0) GID, and we can not simply switch
from one GID to another.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto 149d0416ae18dcfce6e665d66df1349998f8e85a

3 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto e6254c8

3 months ago

Build succeeded.

Commit b4fd03b fixes this pull-request

Pull-Request has been merged by praiskup

3 months ago