PR#410: [frontend] new config REPO_NO_SSL - copr/copr

copr / copr

#410 [frontend] new config REPO_NO_SSL

Merged 5 years ago by praiskup. Opened 5 years ago by praiskup.

Unknown source REPO_NO_SSL into master

Pavel Raiskup • 5 years ago

d5b9b90

file modified

		`@@ -57,6 +57,9 @@`

		`REPO_GPGCHECK = 1`

		`+ # should baseurls in '.repo' files always use http:// links?`
		`+ REPO_NO_SSL = False`
		`+`
		`STORAGE_DIR = "/var/lib/copr/data/srpm_storage/"`

		`LAYOUT_OVERVIEW_HIDE_QUICK_ENABLE = False`

frontend/coprs_frontend/coprs/filters.py

file modified

		`@@ -230,6 +230,8 @@`

		`@app.template_filter("fix_url_https_backend")`
		`def fix_url_https_backend(url):`
		`+ if app.config.get('REPO_NO_SSL', False):`
		`+ return url.replace('https://', 'http://')`
msuchy commented 5 years ago if REPO_NO_SSL is False then use http? I would say the semantic should be opposite. Do that if it is true. Right?
praiskup commented 5 years ago If `app.config.get('REPO_NO_SSL')` returns True then it is replaced. False is only the default.
		`return helpers.fix_protocol_for_backend(url)`

frontend/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_general.py

file modified

+14

		`@@ -719,3 +719,17 @@`
		`# self.tc.get("/coprs/fulltext/?fulltext={}".format("user1/"))`
		`# qargs, qkwargs = mc_render_template.call_args`
		`# assert qkwargs["paginator"].total_count == 5`
		`+`
		`+`
		`+ class TestRepo(CoprsTestCase):`
		`+ @mock.patch.dict("coprs.app.config", {'REPO_NO_SSL': True})`
		`+ @mock.patch.dict("coprs.app.config", {'ENFORCE_PROTOCOL_FOR_BACKEND_URL': "https"})`
		`+ def test_repo_renders_http(self, f_users, f_coprs, f_mock_chroots, f_db):`
		`+ url = "/coprs/{user}/{copr}/repo/{chroot}/{user}-{copr}-{chroot}.repo".format(`
		`+ user = self.u1.username,`
		`+ copr = self.c1.name,`
		`+ chroot = "{}-{}".format(self.mc1.os_release, self.mc1.os_version),`
		`+ )`
		`+ res = self.tc.get(url)`
		`+ assert res.status_code == 200`
		`+ assert 'baseurl=http://' in res.data.decode('utf-8')`

praiskup commented 5 years ago

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463659

msuchy commented on line 6 of frontend/coprs_frontend/coprs/config.py 5 years ago

Can this be rather True/False?

msuchy commented on line 5 of frontend/coprs_frontend/coprs/filters.py 5 years ago

Can you assume some defaults? I.e., get('REPO_NO_SSL', False)

praiskup commented on line 6 of frontend/coprs_frontend/coprs/config.py 5 years ago

It's weird to have True/False here, and 1/0 three lines above. Otherwise I don't mind.

praiskup commented on line 5 of frontend/coprs_frontend/coprs/filters.py 5 years ago

The defaults come from config.py, but I can adjust it here as you propose.

rebased onto 853e8261b98cf1f1448f271975dcba2b702cab3d

5 years ago

rebased onto d5b9b90

5 years ago

praiskup commented 5 years ago

PTAL

msuchy commented 5 years ago

clime commented 5 years ago

Hello, why you are not using already present ENFORCE_PROTOCOL_FOR_BACKEND_URL?

praiskup commented 5 years ago

I don't want to disable ssl globally for backend, that would be overkill since the infrastructure is perfectly OK to run on SSL.

clime commented 5 years ago

E.g. urls for build chroots on a build detail are generated like this:

@property
def result_dir_url(self):
    return urljoin(app.config["BACKEND_BASE_URL"], os.path.join(
        "results", self.build.copr.full_name, self.name, self.result_dir, ""))

so there is no url fixing.

fix_protocol_for_backend (the function using the ENFORCE_ thing) is used for only for yum repo url generation, which should be what you need.

If you want to introduce REPO_NO_SSL as a replacement for ENFORCE_PROTOCOL_FOR_BACKEND_URL, I am fine with that as REPO_NO_SSL is probably a better name.

praiskup commented 5 years ago

I thought that ENFORCE_PROTOCOL_FOR_BACKEND_URL has more global effect, lemme check.

praiskup commented 5 years ago

See get_yum_repos in API, and fix_protocol_for_backend.

praiskup commented 5 years ago

IOW, I don't really want to (even accidentally in future) make the copr infrastructure to communicate over http:// (e.g. rpmbulid <=> backend <=> distgit <=> frontend needs to be fully over SSL). I only want to affect the *.repo file contents.

praiskup commented 5 years ago

Voting says +1 so far, so I could probably merge. I'd do it tomorrow, please suggest
a change or speak up.

msuchy commented on line 6 of frontend/coprs_frontend/coprs/filters.py 5 years ago

if REPO_NO_SSL is False then use http? I would say the semantic should be opposite. Do that if it is true. Right?

praiskup commented on line 6 of frontend/coprs_frontend/coprs/filters.py 5 years ago

If app.config.get('REPO_NO_SSL') returns True then it is replaced. False is only the default.

praiskup commented 5 years ago

No more changes requested, merging.

Pull-Request has been merged by praiskup

5 years ago