Merge #461 NO_AUTH bugfix and improvement

module_build_service/auth.py

@@ -83,9 +83,9 @@ def get_user(request):
     Returns the client's username and groups based on the OIDC token provided.
     """
 
-    if app.config['NO_AUTH']:
+    if app.config['NO_AUTH'] is True:
         log.debug("Authorization is disabled.")
-        return
+        return "anonymous", {"packager"}
 
     _load_secrets()
 

module_build_service/utils.py

@@ -721,17 +721,6 @@ def submit_module_build(username, url, mmd, scm, yaml, optional_params=None):
     return module
 
 
-def validate_optional_params(params):
-    forbidden_params = [k for k in params if k not in models.ModuleBuild.__table__.columns and k not in ["branch"]]
-    if forbidden_params:
-        raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params)))
-
-    forbidden_params = [k for k in params if k.startswith("copr_")]
-    if conf.system != "copr" and forbidden_params:
-        raise ValidationError('The request contains parameters specific to Copr builder: {} even though {} is used'
-                              .format(", ".join(forbidden_params), conf.system))
-
-
 def scm_url_schemes(terse=False):
     """
     Definition of URL schemes supported by both frontend and scheduler.

module_build_service/views.py

@@ -35,7 +35,7 @@ from module_build_service import app, conf, log
 from module_build_service import models, db
 from module_build_service.utils import (
     pagination_metadata, filter_module_builds, submit_module_build_from_scm,
-    submit_module_build_from_yaml, scm_url_schemes, get_scm_url_re, validate_optional_params)
+    submit_module_build_from_yaml, scm_url_schemes, get_scm_url_re)
 from module_build_service.errors import (
     ValidationError, Forbidden, NotFound)
 
@@ -95,67 +95,36 @@ class ModuleBuildAPI(MethodView):
                 raise NotFound('No such module found.')
 
     def post(self):
-        username, groups = module_build_service.auth.get_user(request)
+        if "multipart/form-data" in request.headers.get("Content-Type", ""):
+            handler = YAMLFileHandler(request)
+        else:
+            handler = SCMHandler(request)
 
-        if conf.allowed_groups and not (conf.allowed_groups & groups):
+        if conf.no_auth is True and handler.username == "anonymous" and "owner" in handler.data:
+            handler.username = handler.data["owner"]
+
+        if conf.allowed_groups and not (conf.allowed_groups & handler.groups):
             raise Forbidden("%s is not in any of  %r, only %r" % (
-                username, conf.allowed_groups, groups))
+                handler.username, conf.allowed_groups, handler.groups))
-
-        kwargs = {"username": username}
-        module = (self.post_file(**kwargs) if "multipart/form-data" in request.headers.get("Content-Type", "") else
-                  self.post_scm(**kwargs))
 
+        handler.validate()
+        module = handler.post()
         return jsonify(module.json()), 201
 
-    def post_scm(self, username):
+    def patch(self, id):
+        username, groups = module_build_service.auth.get_user(request)
+
         try:
             r = json.loads(request.get_data().decode("utf-8"))
         except:
             log.error('Invalid JSON submitted')
             raise ValidationError('Invalid JSON submitted')
 
-        if "scmurl" not in r:
+        if "owner" in r:
-            log.error('Missing scmurl')
+            if conf.no_auth is not True:
-            raise ValidationError('Missing scmurl')
+                raise ValidationError("The request contains 'owner' parameter, however NO_AUTH is not allowed")
-
+            elif username == "anonymous":
-        url = r["scmurl"]
+                username = r["owner"]
-        if not any(url.startswith(prefix) for prefix in conf.scmurls):
-            log.error("The submitted scmurl %r is not allowed" % url)
-            raise Forbidden("The submitted scmurl %s is not allowed" % url)
-
-        if not get_scm_url_re().match(url):
-            log.error("The submitted scmurl %r is not valid" % url)
-            raise Forbidden("The submitted scmurl %s is not valid" % url)
-
-        if "branch" not in r:
-            log.error('Missing branch')
-            raise ValidationError('Missing branch')
-
-        branch = r["branch"]
-
-        # python-modulemd expects this to be bytes, not unicode.
-        if isinstance(branch, unicode):
-            branch = branch.encode('utf-8')
-
-        validate_optional_params(r)
-        optional_params = {k: v for k, v in r.items() if k != "scmurl" and k != 'branch'}
-        return submit_module_build_from_scm(username, url, branch, allow_local_url=False, optional_params=optional_params)
-
-    def post_file(self, username):
-        if not conf.yaml_submit_allowed:
-            raise Forbidden("YAML submission is not enabled")
-        validate_optional_params(request.form)
-
-        try:
-            r = request.files["yaml"]
-        except:
-            log.error('Invalid file submitted')
-            raise ValidationError('Invalid file submitted')
-
-        return submit_module_build_from_yaml(username, r.read(), optional_params=request.form.to_dict())
-
-    def patch(self, id):
-        username, groups = module_build_service.auth.get_user(request)
 
         if conf.allowed_groups and not (conf.allowed_groups & groups):
             raise Forbidden("%s is not in any of  %r, only %r" % (
@@ -169,12 +138,6 @@ class ModuleBuildAPI(MethodView):
             raise Forbidden('You are not owner of this build and '
                             'therefore cannot modify it.')
 
-        try:
-            r = json.loads(request.get_data().decode("utf-8"))
-        except:
-            log.error('Invalid JSON submitted')
-            raise ValidationError('Invalid JSON submitted')
-
         if not r.get('state'):
             log.error('Invalid JSON submitted')
             raise ValidationError('Invalid JSON submitted')
@@ -193,6 +156,89 @@ class ModuleBuildAPI(MethodView):
         return jsonify(module.api_json()), 200
 
 
+class BaseHandler(object):
+    def __init__(self, request):
+        self.username, self.groups = module_build_service.auth.get_user(request)
+        self.data = None
+
+    @property
+    def optional_params(self):
+        return {k: v for k, v in self.data.items() if k not in ["owner", "scmurl", "branch"]}
+
+    def validate_optional_params(self):
+        forbidden_params = [k for k in self.data if k not in models.ModuleBuild.__table__.columns
+                            and k not in ["branch"]]
+        if forbidden_params:
+            raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params)))
+
+        forbidden_params = [k for k in self.data if k.startswith("copr_")]
+        if conf.system != "copr" and forbidden_params:
+            raise ValidationError('The request contains parameters specific to Copr builder: {} even though {} is used'
+                                  .format(", ".join(forbidden_params), conf.system))
+
+        if not conf.no_auth and "owner" in self.data:
+            raise ValidationError("The request contains 'owner' parameter, however NO_AUTH is not allowed")
+
+
+class SCMHandler(BaseHandler):
+    def __init__(self, request):
+        super(SCMHandler, self).__init__(request)
+        try:
+            self.data = json.loads(request.get_data().decode("utf-8"))
+        except:
+            log.error('Invalid JSON submitted')
+            raise ValidationError('Invalid JSON submitted')
+
+    def validate(self):
+        if "scmurl" not in self.data:
+            log.error('Missing scmurl')
+            raise ValidationError('Missing scmurl')
+
+        url = self.data["scmurl"]
+        if not any(url.startswith(prefix) for prefix in conf.scmurls):
+            log.error("The submitted scmurl %r is not allowed" % url)
+            raise Forbidden("The submitted scmurl %s is not allowed" % url)
+
+        if not get_scm_url_re().match(url):
+            log.error("The submitted scmurl %r is not valid" % url)
+            raise Forbidden("The submitted scmurl %s is not valid" % url)
+
+        if "branch" not in self.data:
+            log.error('Missing branch')
+            raise ValidationError('Missing branch')
+
+        self.validate_optional_params()
+
+    def post(self):
+        url = self.data["scmurl"]
+        branch = self.data["branch"]
+
+        # python-modulemd expects this to be bytes, not unicode.
+        if isinstance(branch, unicode):
+            branch = branch.encode('utf-8')
+
+        return submit_module_build_from_scm(self.username, url, branch,
+                                            allow_local_url=False, optional_params=self.optional_params)
+
+
+class YAMLFileHandler(BaseHandler):
+    def __init__(self, request):
+        if not conf.yaml_submit_allowed:
+            raise Forbidden("YAML submission is not enabled")
+        super(YAMLFileHandler, self).__init__(request)
+        self.data = request.form.to_dict()
+
+    def validate(self):
+        if "yaml" not in request.files:
+            log.error('Invalid file submitted')
+            raise ValidationError('Invalid file submitted')
+        self.validate_optional_params()
+
+    def post(self):
+        r = request.files["yaml"]
+        return submit_module_build_from_yaml(self.username, r.read(), optional_params=self.optional_params)
+
+
 def register_api_v1():
     """ Registers version 1 of MBS API. """
     module_view = ModuleBuildAPI.as_view('module_builds')

tests/test_auth.py

@@ -104,7 +104,9 @@ class TestAuthModule(unittest.TestCase):
     def test_disable_authentication(self):
         with patch.dict('module_build_service.app.config', {'NO_AUTH': True}, clear=True):
             request = mock.MagicMock()
-            eq_(module_build_service.auth.get_user(request), None)
+            username, groups = module_build_service.auth.get_user(request)
+            eq_(username, "anonymous")
+            eq_(groups, {"packager"})
 
     @patch('module_build_service.auth.client_secrets', None)
     def test_misconfiguring_oidc_client_secrets_should_be_failed(self):

tests/test_views/test_views.py

@@ -32,13 +32,14 @@ from os.path import dirname
 import modulemd as _modulemd
 
 from tests import app, init_data
-from module_build_service.models import ComponentBuild
+from module_build_service.models import ComponentBuild, ModuleBuild
 import module_build_service.scm
 from module_build_service import conf
 
 
 user = ('Homer J. Simpson', set(['packager']))
 other_user = ('some_other_user', set(['packager']))
+anonymous_user = ('anonymous', set(['packager']))
 base_dir = dirname(dirname(__file__))
 cassette_dir = base_dir + '/vcr-request-data/'
 
@@ -649,3 +650,64 @@ class TestViews(unittest.TestCase):
             'The stream "wrong_stream" that is stored in the modulemd does not '
             'match the branch "master"')
         self.assertEquals(data['error'], 'Bad Request')
+
+    @patch('module_build_service.auth.get_user', return_value=user)
+    def test_submit_build_set_owner(self, mocked_get_user):
+        data = {
+            'branch': 'master',
+            'scmurl': 'git://pkgs.stg.fedoraproject.org/modules/'
+                      'testmodule.git?#68931c90de214d9d13feefbd35246a81b6cb8d49',
+            'owner': 'foo',
+        }
+        rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(data))
+        result = json.loads(rv.data)
+        self.assertEquals(result['status'], 400)
+        self.assertIn("The request contains 'owner' parameter", result['message'])
+
+    @patch('module_build_service.auth.get_user', return_value=anonymous_user)
+    @patch('module_build_service.scm.SCM')
+    @patch("module_build_service.config.Config.no_auth", new_callable=PropertyMock, return_value=True)
+    def test_submit_build_no_auth_set_owner(self, mocked_conf, mocked_scm, mocked_get_user):
+        MockedSCM(mocked_scm, 'testmodule', 'testmodule.yaml',
+                  '620ec77321b2ea7b0d67d82992dda3e1d67055b4')
+
+        data = {
+            'branch': 'master',
+            'scmurl': 'git://pkgs.stg.fedoraproject.org/modules/'
+                      'testmodule.git?#68931c90de214d9d13feefbd35246a81b6cb8d49',
+            'owner': 'foo',
+        }
+        rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(data))
+        result = json.loads(rv.data)
+
+        build = ModuleBuild.query.filter(ModuleBuild.id == result['id']).one()
+        self.assertTrue(build.owner == result['owner'] == 'foo')
+
+    @patch('module_build_service.auth.get_user', return_value=anonymous_user)
+    @patch('module_build_service.scm.SCM')
+    @patch("module_build_service.config.Config.no_auth", new_callable=PropertyMock)
+    def test_patch_set_different_owner(self, mocked_no_auth, mocked_scm, mocked_get_user):
+        MockedSCM(mocked_scm, 'testmodule', 'testmodule.yaml',
+                  '620ec77321b2ea7b0d67d82992dda3e1d67055b4')
+
+        mocked_no_auth.return_value = True
+        data = {
+            'branch': 'master',
+            'scmurl': 'git://pkgs.stg.fedoraproject.org/modules/'
+                      'testmodule.git?#68931c90de214d9d13feefbd35246a81b6cb8d49',
+            'owner': 'foo',
+        }
+        rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(data))
+        r1 = json.loads(rv.data)
+
+        url = '/module-build-service/1/module-builds/' + str(r1['id'])
+        r2 = self.client.patch(url, data=json.dumps({'state': 'failed'}))
+        self.assertEquals(r2.status_code, 403)
+
+        r3 = self.client.patch(url, data=json.dumps({'state': 'failed', 'owner': 'foo'}))
+        self.assertEquals(r3.status_code, 200)
+
+        mocked_no_auth.return_value = False
+        r3 = self.client.patch(url, data=json.dumps({'state': 'failed', 'owner': 'foo'}))
+        self.assertEquals(r3.status_code, 400)
+        self.assertIn("The request contains 'owner' parameter", json.loads(r3.data)['message'])