PR#515: Add config option for allowing custom scmurls - fm-orchestrator

fm-orchestrator

#515 Add config option for allowing custom scmurls

Merged 6 years ago by jkaluza. Opened 7 years ago by frostyx.

frostyx/fm-orchestrator allow_custom_urls into master

Rename ALLOW_CUSTOM_URLS to ALLOW_CUSTOM_SCMURLS

Jakub Kadlčík • 6 years ago

a860847

Add VCR file for test_submit_custom_scmurl

Jakub Kadlčík • 6 years ago

dde4e4b

Test allow_custom_urls config option

Jakub Kadlčík • 7 years ago

03f8a54

Add config option for allowing custom scmurls

Jakub Kadlčík • 7 years ago

6dde52f

conf/config.py

file modified

		`@@ -47,6 +47,8 @@`
		`# and be in the build state at a time. Set this to 0 for no restrictions`
		`NUM_CONSECUTIVE_BUILDS = 5`

		`+ ALLOW_CUSTOM_SCMURLS = False`
		`+`
		`RPMS_DEFAULT_REPOSITORY = 'git://pkgs.fedoraproject.org/rpms/'`
		`RPMS_ALLOW_REPOSITORY = False`
		`RPMS_DEFAULT_CACHE = 'http://pkgs.fedoraproject.org/repo/pkgs/'`

module_build_service/config.py

file modified

		`@@ -171,6 +171,10 @@`
		`'type': list,`
		`'default': ['module'],`
		`'desc': 'List of allowed koji tag prefixes.'},`
		`+ 'allow_custom_scmurls': {`
		`+ 'type': bool,`
		`+ 'default': False,`
		`+ 'desc': 'Allow custom scmurls.'},`
		`'rpms_default_repository': {`
		`'type': str,`
		`'default': 'git://pkgs.fedoraproject.org/rpms/',`

module_build_service/views.py

file modified

+2 -1

		`@@ -195,7 +195,8 @@`
		`raise ValidationError('Missing scmurl')`

		`url = self.data["scmurl"]`
		`- if not any(url.startswith(prefix) for prefix in conf.scmurls):`
		`+ allowed_prefix = any(url.startswith(prefix) for prefix in conf.scmurls)`
		`+ if not conf.allow_custom_scmurls and not allowed_prefix:`
		`log.error("The submitted scmurl %r is not allowed" % url)`
		`raise Forbidden("The submitted scmurl %s is not allowed" % url)`

tests/test_views/test_views.py

file modified

+22

		`@@ -744,3 +744,25 @@`
		`data['message'])`
		`self.assertEquals(data['status'], 422)`
		`self.assertEquals(data['error'], 'Unprocessable Entity')`
		`+`
		`+ @patch('module_build_service.auth.get_user', return_value=user)`
		`+ @patch('module_build_service.scm.SCM')`
		`+ @patch("module_build_service.config.Config.allow_custom_scmurls", new_callable=PropertyMock)`
		`+ def test_submit_custom_scmurl(self, allow_custom_scmurls, mocked_scm, mocked_get_user):`
		`+ MockedSCM(mocked_scm, 'testmodule', 'testmodule.yaml',`
		`+ '620ec77321b2ea7b0d67d82992dda3e1d67055b4')`
		`+`
		`+ def submit(scmurl):`
		`+ return self.client.post('/module-build-service/1/module-builds/', data=json.dumps(`
		`+ {'branch': 'master', 'scmurl': scmurl}))`
		`+`
		`+ allow_custom_scmurls.return_value = False`
		`+ res1 = submit('git://some.custom.url.org/modules/testmodule.git?#68931c9')`
		`+ data = json.loads(res1.data)`
		`+ self.assertEquals(data['status'], 403)`
		`+ self.assertTrue(data['message'].startswith('The submitted scmurl'))`
		`+ self.assertTrue(data['message'].endswith('is not allowed'))`
		`+`
		`+ allow_custom_scmurls.return_value = True`
		`+ res2 = submit('git://some.custom.url.org/modules/testmodule.git?#68931c9')`
		`+ self.assertEquals(res2.status_code, 201)`

tests/vcr-request-data/test_views.TestViews.test_submit_custom_scmurl

file added

+3565

The added file is too large to be shown here, see it at: tests/vcr-request-data/test_views.TestViews.test_submit_custom_scmurl

frostyx commented 7 years ago

See an issue #513 for the details

mprahl commented 7 years ago

@frostyx @jkaluza: what do you think about making scmurls a regex pattern instead? This would solve this issue and provide more flexibility.

frostyx commented 7 years ago

@mprahl hehe, I suggested it in the #513 issue, but personally I prefer config option for this and @jkaluza gave +1 for that option too. I don't really care though, if you guys agree on regex as a better way, I will re-implement it.

mprahl commented 7 years ago

@frostyx I'm okay with this approach too. Do you want to write a unit test against this?

frostyx commented 7 years ago

Sure, I can

jkaluza commented 7 years ago

+1 once the unit tests are here.

1 new commit added

Test allow_custom_urls config option

7 years ago

frostyx commented 7 years ago

Ok guys, sorry for delay. There are some tests

mprahl commented on line 25 of tests/test_views/test_views.py 7 years ago

Does this test end up reaching out to the internet?

jkaluza commented 7 years ago

Please include tests/vcr-request-data/test_views.TestViews.test_submit_custom_scmurl in the commit. It is auto-generated by the vcrpy when you run this test.

jkaluza commented on line 25 of tests/test_views/test_views.py 7 years ago

It does not. This MBS API call does only text-based validation of scmurl and stores it in database. The real validation using git is done on backend, but backend is not executed in this test. So it should be OK.

1 new commit added

Add VCR file for test_submit_custom_scmurl

6 years ago

frostyx commented 6 years ago

Please include tests/vcr-request-data/... in the commit. It is auto-generated by the vcrpy when you run this test.

Ah, sorry. I always forget to do this. Added.

jkaluza commented 6 years ago

Hm, just wanted to merge the PR, but got one more idea... Could this be renamed to "ALLOW_CUSTOM_SCMURLS". We have "SCMURLS" option, so naming it like ALLOW_CUSTOM_SCMURLS would be better I think.

Sorry for that, I know it was me suggesting ALLOW_CUSTOM_URLS in the beginning :).

1 new commit added