From a51ef6a02d08551ae966ef6dac016673db69ba1f Mon Sep 17 00:00:00 2001
From: Jan Kaluza <jkaluza@redhat.com>
Date: Oct 05 2017 20:12:54 +0000
Subject: Move krb_context from freshmaker.handlers to freshmaker.utils and ensure krb_login is called for any koji_service call when krb principal is set.


---

diff --git a/freshmaker/config.py b/freshmaker/config.py
index 6c940fb..59c949a 100644
--- a/freshmaker/config.py
+++ b/freshmaker/config.py
@@ -256,7 +256,7 @@ class Config(object):
             'desc': 'Whether to acquire credential cache from a client keytab.'},
         'krb_auth_principal': {
             'type': str,
-            'default': True,
+            'default': "",
             'desc': 'Principal used to acquire credential cache, which must be'
                     ' present in specified client keytab.'},
         'krb_auth_client_keytab': {
diff --git a/freshmaker/handlers/__init__.py b/freshmaker/handlers/__init__.py
index fc45be8..a5ae417 100644
--- a/freshmaker/handlers/__init__.py
+++ b/freshmaker/handlers/__init__.py
@@ -32,7 +32,6 @@ from freshmaker.mbs import MBS
 from freshmaker.models import ArtifactBuildState
 from freshmaker.types import ArtifactType
 from freshmaker.models import ArtifactBuild, Event
-from krbcontext import krbContext
 
 from freshmaker.odcsclient import ODCS
 from freshmaker.odcsclient import AuthMech
@@ -63,23 +62,6 @@ class BaseHandler(object):
         """
         raise NotImplementedError()
 
-    @property
-    def krb_context(self):
-        if conf.krb_auth_use_keytab:
-            krb_ctx_opts = {
-                'using_keytab': conf.krb_auth_use_keytab,
-                'principal': conf.krb_auth_principal,
-                'keytab_file': conf.krb_auth_client_keytab,
-                'ccache_file': conf.krb_auth_ccache_file,
-            }
-        else:
-            krb_ctx_opts = {
-                'principal': conf.krb_auth_principal,
-                'ccache_file': conf.krb_auth_ccache_file,
-            }
-
-        return krbContext(**krb_ctx_opts)
-
     def build_module(self, name, branch, rev):
         """
         Build a module in MBS.
@@ -197,19 +179,6 @@ class ContainerBuildHandler(BaseHandler):
         :rtype: int
         """
         with koji_service(profile=conf.koji_profile, logger=log) as service:
-            log.debug('Logging into %s with Kerberos authentication.',
-                      service.server)
-
-            proxyuser = conf.koji_build_owner if conf.koji_proxyuser else None
-
-            with self.krb_context:
-                service.krb_login(proxyuser=proxyuser)
-
-            # We are not logged in in dry run mode...
-            if not conf.dry_run and not service.logged_in:
-                log.error('Could not login server %s', service.server)
-                return None
-
             log.info('Building container from source: %s, '
                      'release=%r, parent=%r, target=%r',
                      scm_url, release, koji_parent_build, target)
diff --git a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py
index 789ffcf..f6794b4 100644
--- a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py
+++ b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py
@@ -38,6 +38,7 @@ from freshmaker.errata import Errata
 from freshmaker.types import ArtifactType, ArtifactBuildState
 from freshmaker.models import Event
 from freshmaker.consumer import work_queue_put
+from freshmaker.utils import krb_context
 
 from odcs.client.odcs import ODCS
 from odcs.client.odcs import AuthMech
@@ -200,7 +201,7 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler):
         odcs = ODCS(conf.odcs_server_url, auth_mech=AuthMech.Kerberos,
                     verify_ssl=conf.odcs_verify_ssl)
         if not conf.dry_run:
-            with self.krb_context:
+            with krb_context():
                 new_compose = odcs.new_compose(
                     compose_source, 'tag', packages=packages)
         else:
diff --git a/freshmaker/kojiservice.py b/freshmaker/kojiservice.py
index 5b28081..067da6b 100644
--- a/freshmaker/kojiservice.py
+++ b/freshmaker/kojiservice.py
@@ -28,6 +28,7 @@ import re
 from freshmaker import log, conf
 from freshmaker.consumer import work_queue_put
 from freshmaker.events import BrewContainerTaskStateChangeEvent
+from freshmaker.utils import krb_context
 
 
 class KojiService(object):
@@ -162,7 +163,7 @@ class KojiService(object):
 
 
 @contextlib.contextmanager
-def koji_service(profile=None, logger=None):
+def koji_service(profile=None, logger=None, login=True):
     """A Koji service context manager that could be used with with
 
     Example::
@@ -179,6 +180,24 @@ def koji_service(profile=None, logger=None):
             ...
     """
     service = KojiService(profile=profile)
+
+    if login:
+        if not conf.krb_auth_principal:
+            log.error("Cannot login to Koji, krb_auth_principal not set")
+        else:
+            log.debug('Logging into %s with Kerberos authentication.',
+                      service.server)
+
+            proxyuser = conf.koji_build_owner if conf.koji_proxyuser else None
+
+            with krb_context():
+                service.krb_login(proxyuser=proxyuser)
+
+            # We are not logged in in dry run mode...
+            if not conf.dry_run and not service.logged_in:
+                log.error('Could not login server %s', service.server)
+                yield None
+
     try:
         yield service
     finally:
diff --git a/freshmaker/utils.py b/freshmaker/utils.py
index 5709531..3723a93 100644
--- a/freshmaker/utils.py
+++ b/freshmaker/utils.py
@@ -32,6 +32,24 @@ import tempfile
 import time
 
 from freshmaker import conf
+from krbcontext import krbContext
+
+
+def krb_context():
+    if conf.krb_auth_use_keytab:
+        krb_ctx_opts = {
+            'using_keytab': conf.krb_auth_use_keytab,
+            'principal': conf.krb_auth_principal,
+            'keytab_file': conf.krb_auth_client_keytab,
+            'ccache_file': conf.krb_auth_ccache_file,
+        }
+    else:
+        krb_ctx_opts = {
+            'principal': conf.krb_auth_principal,
+            'ccache_file': conf.krb_auth_ccache_file,
+        }
+
+    return krbContext(**krb_ctx_opts)
 
 
 def load_class(location):
diff --git a/tests/test_errata_advisory_state_changed.py b/tests/test_errata_advisory_state_changed.py
index 9ea008c..b86f4f5 100644
--- a/tests/test_errata_advisory_state_changed.py
+++ b/tests/test_errata_advisory_state_changed.py
@@ -453,8 +453,7 @@ class TestPrepareYumRepo(unittest.TestCase):
            'ErrataAdvisoryRPMsSignedHandler._get_compose_source')
     @patch('time.sleep')
     @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Errata')
-    @patch('freshmaker.handlers.BaseHandler.krb_context',
-           new_callable=PropertyMock)
+    @patch('freshmaker.utils.krbContext')
     def test_get_repo_url_when_succeed_to_generate_compose(
             self, krb_context, errata, sleep, _get_compose_source,
             _get_packages_for_compose, ODCS):
@@ -497,7 +496,7 @@ class TestPrepareYumRepo(unittest.TestCase):
            'ErrataAdvisoryRPMsSignedHandler._get_compose_source')
     @patch('time.sleep')
     @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Errata')
-    @patch('freshmaker.handlers.BaseHandler.krb_context',
+    @patch('freshmaker.kojiservice.krb_context',
            new_callable=PropertyMock)
     def test_get_repo_url_packages_in_multiple_tags(
             self, krb_context, errata, sleep, _get_compose_source,
@@ -528,7 +527,7 @@ class TestPrepareYumRepo(unittest.TestCase):
            'ErrataAdvisoryRPMsSignedHandler._get_compose_source')
     @patch('time.sleep')
     @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Errata')
-    @patch('freshmaker.handlers.BaseHandler.krb_context',
+    @patch('freshmaker.kojiservice.krb_context',
            new_callable=PropertyMock)
     def test_get_repo_url_packages_not_found_in_tag(
             self, krb_context, errata, sleep, _get_compose_source,
diff --git a/tests/test_git_dockerfile_change_handler.py b/tests/test_git_dockerfile_change_handler.py
index 15583a2..9c9a9ea 100644
--- a/tests/test_git_dockerfile_change_handler.py
+++ b/tests/test_git_dockerfile_change_handler.py
@@ -25,7 +25,7 @@ import unittest
 import fedmsg.config
 
 from mock import patch
-from mock import MagicMock
+from mock import MagicMock, PropertyMock
 
 from freshmaker import db, models
 from freshmaker.consumer import FreshmakerConsumer
@@ -62,9 +62,11 @@ class GitDockerfileChangeHandlerTest(BaseTestCase):
 
     @patch('koji.read_config')
     @patch('koji.ClientSession')
-    @patch('freshmaker.handlers.krbContext')
+    @patch('freshmaker.utils.krbContext')
+    @patch("freshmaker.config.Config.krb_auth_principal",
+           new_callable=PropertyMock, return_value="user@example.com")
     def test_rebuild_if_dockerfile_changed(
-            self, krbContext, ClientSession, read_config):
+            self, auth_principal, krbContext, ClientSession, read_config):
         read_config.return_value = {
             'server': 'https://localhost/kojihub',
             'krb_rdns': False,
@@ -117,7 +119,9 @@ class GitDockerfileChangeHandlerTest(BaseTestCase):
 
     @patch('koji.read_config')
     @patch('koji.ClientSession')
-    def test_ensure_do_nothing_if_fail_to_login_koji(self, ClientSession, read_config):
+    @patch("freshmaker.config.Config.krb_auth_principal",
+           new_callable=PropertyMock, return_value="user@example.com")
+    def test_ensure_do_nothing_if_fail_to_login_koji(self, auth_principal, ClientSession, read_config):
         ClientSession.return_value.krb_login.side_effect = RuntimeError
         read_config.return_value = {
             'server': 'https://localhost/kojihub',
diff --git a/tests/test_handler.py b/tests/test_handler.py
index 3bf0837..fc51b3a 100644
--- a/tests/test_handler.py
+++ b/tests/test_handler.py
@@ -24,7 +24,7 @@
 
 import json
 
-from mock import patch
+from mock import patch, PropertyMock
 from unittest import TestCase
 
 from freshmaker import db
@@ -49,15 +49,17 @@ class TestKrbContextPreparedForBuildContainer(TestCase):
     """Test krb_context for BaseHandler.build_container"""
 
     def setUp(self):
-        self.koji_service = patch('freshmaker.handlers.koji_service')
+        self.koji_service = patch('freshmaker.kojiservice.KojiService')
         self.koji_service.start()
 
     def tearDown(self):
         self.koji_service.stop()
 
-    @patch('freshmaker.handlers.conf')
-    @patch('freshmaker.handlers.krbContext')
-    def test_prepare_with_keytab(self, krbContext, conf):
+    @patch('freshmaker.utils.conf')
+    @patch('freshmaker.utils.krbContext')
+    @patch("freshmaker.config.Config.krb_auth_principal",
+           new_callable=PropertyMock, return_value="user@example.com")
+    def test_prepare_with_keytab(self, auth_principal, krbContext, conf):
         conf.krb_auth_use_keytab = True
         conf.krb_auth_principal = 'freshmaker/hostname@REALM'
         conf.krb_auth_client_keytab = '/etc/freshmaker.keytab'
@@ -73,9 +75,11 @@ class TestKrbContextPreparedForBuildContainer(TestCase):
             ccache_file='/tmp/freshmaker_cc',
         )
 
-    @patch('freshmaker.handlers.conf')
-    @patch('freshmaker.handlers.krbContext')
-    def test_prepare_with_normal_user_credential(self, krbContext, conf):
+    @patch('freshmaker.utils.conf')
+    @patch('freshmaker.utils.krbContext')
+    @patch("freshmaker.config.Config.krb_auth_principal",
+           new_callable=PropertyMock, return_value="user@example.com")
+    def test_prepare_with_normal_user_credential(self, auth_principal, krbContext, conf):
         conf.krb_auth_use_keytab = False
         conf.krb_auth_principal = 'somebody@REALM'
         conf.krb_auth_ccache_file = '/tmp/freshmaker_cc'
@@ -150,7 +154,7 @@ class TestBuildFirstBatch(TestCase):
 
     @patch('freshmaker.handlers.ODCS')
     @patch('koji.ClientSession')
-    @patch('freshmaker.handlers.krbContext')
+    @patch('freshmaker.utils.krbContext')
     def test_build_first_batch(self, krb, ClientSession, ODCS):
         """
         Tests that only PLANNED images without a parent are submitted to

