pagure-dist-git

Clone
Source Code
GIT

#88 Drop two early return statements that by-pass the check for PR only
Merged 5 years ago by pingou. Opened 5 years ago by pingou.
fix_pr_only  into  master

file modified
+26 -15 
@@ -199,20 +199,26 @@ 
 

          self.debug("SIG memberships: %s" % user_sigs)
 

          self.debug("RCM: %s" % is_rcm)
 

  

+         # Quick reminder about the protected_namespace: git.centos.org is
 

+         # hosting in one pagure instance a "regular" git forge as well as
 

+         # a dist-git instance. So they need to protect the dist-git
 

+         # namespaces via the dist-git specific checks while the other
 

+         # namespaces go via the regular checks.
 

+ 

          # We have data, start the actual ACL checking
 

          if (
 

              repotype == "main"
 

              and not project.is_fork
 

              and project.namespace in self.protected_namespaces
 

          ):
 

-             # In the protected namespace, we want to make sure we don't
 

-             # trample on blacklisted content.
 

+             # In the protected namespace, we want to make sure that we block
 

+             # blacklisted branches.
 

              for entry in self.blacklists:
 

                  if entry.match(refname):
 

                      self.info("Ref %s is blocked" % refname)
 

                      return False
 

  

-             # Allow RCM push
 

+             # Allow RCM/releng to push regardless
 

              if is_rcm:
 

                  for refre in self.rcm_branches:
 

                      if refre.match(refname):
 
@@ -228,37 +234,42 @@ 
 

                          self.debug("SIG push")
 

                          return True
 

  

-             # For Fedora, allow supported branches
 

+             # For Fedora, allow supported branches, these are the active
 

+             # branches in PDC
 

              is_supported = self.is_supported_branch(project, refname)
 

              if is_supported is False:
 

                  self.info("Branch %s is unsupported" % refname)
 

                  return False
 

              elif is_supported is True:
 

                  self.debug("Branch %s is supported" % refname)
 

-                 return is_committer
 

              else:
 

                  self.debug("No supported status available")
 

  

+             # This allows to block anything that is not allowed, so no
 

+             # random branch creation.
 

              if self.block_unspecified:
 

                  self.info(
 

                      "Access to namespace %s is restricted" % project.namespace
 

                  )
 

                  return False
 

  

-             # Block second level blacklists
 

-             for entry in self.unspecified_blacklist:
 

-                 if entry.match(refname):
 

-                     self.info("Unspecified ref %s is blocked" % refname)
 

-                     return False
 

+             # For branches that are not explicitely active in PDC, check
 

+             # if the user is allowed to create/push to them.
 

+             if not is_supported:
 

+                 for entry in self.unspecified_blacklist:
 

+                     if entry.match(refname):
 

+                         self.info("Unspecified ref %s is blocked" % refname)
 

+                         return False
 

  

              # For unspecified refs, they can push if they're a committer
 

              self.debug("Unspecified branch push")
 

-             return is_committer
 

  

-         # This is outside of the strongly protected namespaces
 

-         if repotype == "main":
 

+         # This is applicable to all namespace, protected or not
 

+ 

+         if repotype == "main" and not is_rcm:
 

              # If this project has PRs only on, or PRs are globally enforced and
 

              # this is not a fork, only allow pushing if this is a PR merge.
 

+             # However, RCM/releng is allowed to by-pass the PR only model
 

              pr_only = project.settings.get("pull_request_access_only", False)
 

              if (
 

                  pr_only or (self.global_pr_only and not project.is_fork)
 
@@ -266,9 +277,9 @@ 
 

                  self.info("A pull request is required for this branch")
 

                  return False
 

  

-         # This is an unprotected namespace, let's allow committers
 

+         # Allow committers to commit
 

          if is_committer:
 

-             self.debug("Committer push to unprotected")
 

+             self.debug("Committer push")
 

              return True
 

  

          # If all else fails, deny

file modified
+29 -6 
@@ -85,6 +85,7 @@ 
 

          super(DistGitAuthTests, self).setUp()
 

  

          pagure.config.config['ACL_DEBUG'] = True
 

+         pagure.config.config['EXTERNAL_COMMITTER'] = {"relenggroup": {}}
 

          pagure.config.config.update(self.dga_config)
 

  

          self.dga = dist_git_auth.DistGitAuth()
 
@@ -288,7 +289,7 @@ 
 

              )
 

          )
 

  

-         self.expect_info_msg("Committer push to unprotected")
 

+         self.expect_info_msg("Committer push")
 

  

      def test_unprotected_non_committer(self):
 

          project = self.create_namespaced_project('unprotected', 'test')
 
@@ -329,7 +330,7 @@ 
 

              )
 

          )
 

  

-         self.expect_info_msg("Committer push to unprotected")
 

+         self.expect_info_msg("Committer push")
 

  

      def test_unprotected_pr_required_no_pr(self):
 

          project = self.create_namespaced_project('unprotected', 'test')
 
@@ -352,6 +353,27 @@ 
 

  

          self.expect_info_msg("A pull request is required for this branch")
 

  

+     def test_unprotected_pr_required_no_pr_cvsadmin(self):
 

+         project = self.create_namespaced_project('unprotected', 'test')
 

+         self.dga.global_pr_only = True
 

+ 

+         self.assertTrue(
 

+             self.dga.check_acl(
 

+                 self.session,
 

+                 project=project,
 

+                 username="releng",
 

+                 refname="refs/heads/mywip",
 

+                 pull_request=None,
 

+                 repodir=None,
 

+                 repotype='main',
 

+                 revfrom=None,
 

+                 revto=None,
 

+                 is_internal=False,
 

+             )
 

+         )
 

+ 

+         self.expect_info_msg("Committer push")
 

+ 

      def test_unprotected_pr_required_requests(self):
 

          project = self.create_namespaced_project('unprotected', 'test')
 

          self.dga.global_pr_only = True
 
@@ -371,8 +393,8 @@ 
 

              )
 

          )
 

  

-         self.expect_info_msg("Committer push to unprotected")
 

-     

+         self.expect_info_msg("Committer push")
 

+ 

      def test_unprotected_pr_required_repo_pr_only_no_pr(self):
 

          settings = {"pull_request_access_only": True}
 

          project = self.create_namespaced_project('unprotected', 'test', settings=settings)
 
@@ -393,7 +415,7 @@ 
 

          )
 

  

          self.expect_info_msg("A pull request is required for this branch")
 

-     

+ 

      def test_unprotected_pr_required_repo_pr_only(self):
 

          settings = {"pull_request_access_only": True}
 

          project = self.create_namespaced_project(
 
@@ -416,7 +438,8 @@ 
 

              )
 

          )
 

  

-         self.expect_info_msg("Committer push to unprotected")
 

+         self.expect_info_msg("Committer push")
 

+ 

  

  class DistGitAuthTestsFedora(DistGitAuthTests):
 

      dga_config = {

Basically, we were returning True before we had a chance to check if
the project allowed to direct commits vs enforcing a PR only workflow.
RCM and releng are not concerned by this though.

Signed-off-by: Pierre-Yves Chibon pingou@pingoured.fr

rebased onto a99a426
5 years ago

I don't understand the code in question deeply enough, but I can test this on staging. Let me know once it is there.

1 new commit added

  • Expand on the documentation in the code
5 years ago

2 new commits added

  • Expand on the documentation in the code
  • Drop two early return statements that by-pass the check for PR only
5 years ago

spaces on a blank line?

I don't understand the code in question deeply enough, but I can test this on staging. Let me know once it is there.

It is already there :)

removed yes, my IDE does it automatically on save :]

:thumbsup: 

[git (master)]$ git commit --allow-empty
[master c4d96d5] Test 123
[git (master)]$ git remote -v
origin  ssh://churchyard@pkgs.stg.fedoraproject.org/rpms/git.git (fetch)
origin  ssh://churchyard@pkgs.stg.fedoraproject.org/rpms/git.git (push)
[git (master)]$ git push 
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 188 bytes | 188.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0)
remote: Performing pre-check...
remote: Pre-check results in
remote: Welcome to repoSpanner 0.5+11.c6a026e428baaa9b34c8fd54a6f1c0a6f2ae062e.el7.infra, node fedora01.rpms.stg.fedoraproject.org
remote: Delta resolving finished
remote: Validating objects...
remote: Objects validated
remote: Finishing hook runner preparation...
remote: Telling hook runner to grab new contents...
remote: Running pre-receive hook...
remote: Protected namespaces: ['rpms', 'modules', 'container']
remote: Blocking unspecified refs: False
remote: Blacklists: [<_sre.SRE_Pattern object at 0x7fad9d7de7b0>]
remote: User: User: 232 - name churchyard
remote: User groups: set([u'modularity-wg', u'provenpackager', u'packager', u'cvsadmin', u'python-sig'])
remote: Committer: True
remote: SIG memberships: set([])
remote: RCM: False
remote: Branch refs/heads/master is supported
remote: Unspecified branch push
remote: A pull request is required for this branch
remote: Denied push for ref 'refs/heads/master' for user 'churchyard'
remote: All changes have been rejected
remote: Hook returned error
remote: ERR Pre-receive hook refused push
error: remote unpack failed: FAIL: Pre-receive hook refused push
To ssh://pkgs.stg.fedoraproject.org/rpms/git.git
 ! [remote rejected] master -> master (unpack FAIL: Pre-receive hook refused push)
error: failed to push some refs to 'ssh://churchyard@pkgs.stg.fedoraproject.org/rpms/git.git'

It seems to work. However do, cvsadmins get an exception? I'm fine if we don't.

Also, the error doesn't really seem to be actionable.

do, cvsadmins get an exception? I'm fine if we don't.

Yes they do, releng/cvsadmins must be able to push at all time :)

Also, the error doesn't really seem to be actionable.

It's definitely a bit hidden in the verbose output but it's there:
remote: A pull request is required for this branch

Yes they do, releng/cvsadmins must be able to push at all time :)

In that case it doesn't work :(

Also, the error doesn't really seem to be actionable.

It's definitely a bit hidden in the verbose output but it's there:
remote: A pull request is required for this branch

Oh, right, sorry about that. Indeed it is hidden quite well.
Edited 5 years ago by churchyard

Yes they do, releng/cvsadmins must be able to push at all time :)

In that case it doesn't work :(

Are you in that group in staging? (FAS staging) if you are, could you try again? I've adjusted the configuration a bit.

Is this good enough?

remote: User groups: set([u'modularity-wg', u'provenpackager', u'packager', u'cvsadmin', u'python-sig'])

Trying again.

 
$ git push 
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 188 bytes | 188.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0)
remote: Performing pre-check...
remote: Pre-check results in
remote: Welcome to repoSpanner 0.5+11.c6a026e428baaa9b34c8fd54a6f1c0a6f2ae062e.el7.infra, node fedora01.rpms.stg.fedoraproject.org
remote: Delta resolving finished
remote: Validating objects...
remote: Objects validated
remote: Finishing hook runner preparation...
remote: Telling hook runner to grab new contents...
remote: Running pre-receive hook...
remote: Protected namespaces: ['rpms', 'modules', 'container']
remote: Blocking unspecified refs: False
remote: Blacklists: [<_sre.SRE_Pattern object at 0x7f3c3c9187b0>]
remote: User: User: 232 - name churchyard
remote: User groups: set([u'modularity-wg', u'provenpackager', u'packager', u'cvsadmin', u'python-sig'])
remote: Committer: True
remote: SIG memberships: set([])
remote: RCM: False
remote: Branch refs/heads/master is supported
remote: Unspecified branch push
remote: A pull request is required for this branch
remote: Denied push for ref 'refs/heads/master' for user 'churchyard'
remote: All changes have been rejected
remote: Hook returned error
remote: ERR Pre-receive hook refused push
error: remote unpack failed: FAIL: Pre-receive hook refused push
To ssh://pkgs.stg.fedoraproject.org/rpms/git.git
 ! [remote rejected] master -> master (unpack FAIL: Pre-receive hook refused push)
error: failed to push some refs to 'ssh://churchyard@pkgs.stg.fedoraproject.org/rpms/git.git'

Are you in that group in staging? (FAS staging)

Just checked, you are :)

remote: RCM: False

wich comes from

https://pagure.io/pagure-dist-git/blob/a99a426ce05121d543db709457d531ded90dc143/f/dist_git_auth.py#_190:

is_rcm = bool(self.rcm_group and self.rcm_group in usergroups)

and rcm_group...

self.rcm_group = pagure_config.get("RCM_GROUP")

and RCM_GROUP... https://infrastructure.fedoraproject.org/cgit/ansible.git/tree/roles/distgit/pagure/templates/pagure_shared.cfg#n69:

RCM_GROUP = 'relenggroup'

so, looking code and config on ansible, cvsadmin should be blocked, and it is

and we'll need to touch the is_rcm check to modify it to accept lists if cvsadmin should be able to push on this case
Edited 5 years ago by jlanda

1 new commit added

  • Allow RCM/releng to by-pass a PR-only workflow
5 years ago

3 new commits added

  • Allow RCM/releng to by-pass a PR-only workflow
  • Expand on the documentation in the code
  • Drop two early return statements that by-pass the check for PR only
5 years ago

:thumbsup:

Thanks for the review @jlanda and your help testing it in staging @churchyard !

Pull-Request has been merged by pingou
5 years ago
Metadata
None
No Tags
Changes Summary 2
+26 -15
file changed
dist_git_auth.py
+29 -6
file changed
dist_git_auth_tests.py
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.