From 49623431d026347fcf8c1a4b923b36381c6bcb16 Mon Sep 17 00:00:00 2001 From: Lukas Holecek Date: Tue, 23 May 2023 13:50:21 +0200 Subject: [PATCH] Fix submitting form after an interval Also shows a link to the new waiver after submitted. JIRA: RHELWF-9086, RHELWF-9157 --- tests/test_auth.py | 109 +++++++++++++++++++++++++---- waiverdb/api_v1.py | 61 ++++++++++++---- waiverdb/templates/new_waiver.html | 5 +- 3 files changed, 147 insertions(+), 28 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 4c875f6..d48cedb 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -10,12 +10,30 @@ import flask_oidc from flask import g +WAIVER_DATA = { + 'subject_type': 'koji_build', + 'subject_identifier': 'glibc-2.26-27.fc27', + 'testcase': 'testcase1', + 'product_version': 'fool-1', + 'waived': True, + 'comment': 'it broke', +} +WAIVER_PARAMS = '&'.join( + f'{k}={v}' + for k, v in WAIVER_DATA.items() + if isinstance(v, str) +) + @pytest.fixture def oidc_token(app): with mock.patch.object(flask_oidc.OpenIDConnect, '_get_token_info') as mocked: mocked.return_value = { - 'active': True, 'username': 'testuser', 'scope': 'openid waiverdb_scope'} + 'active': True, + 'username': 'testuser', + 'preferred_username': 'testuser', + 'scope': 'openid waiverdb_scope', + } with app.app_context(): g.oidc_id_token = mocked() yield g.oidc_id_token @@ -35,17 +53,9 @@ def permissions(): @pytest.mark.usefixtures('enable_kerberos') class TestGSSAPIAuthentication(object): - waiver_data = { - 'subject': {'type': 'koji_build', 'item': 'glibc-2.26-27.fc27'}, - 'testcase': 'testcase1', - 'product_version': 'fool-1', - 'waived': True, - 'comment': 'it broke', - } - def test_unauthorized(self, client, monkeypatch): monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab') - r = client.post('/api/v1.0/waivers/', data=json.dumps(self.waiver_data), + r = client.post('/api/v1.0/waivers/', data=json.dumps(WAIVER_DATA), content_type='application/json') assert r.status_code == 401 assert r.headers.get('www-authenticate') == 'Negotiate' @@ -61,7 +71,7 @@ def test_authorized(self, client, monkeypatch): monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab') headers = {'Authorization': 'Negotiate %s' % b64encode(b"CTOKEN").decode()} - r = client.post('/api/v1.0/waivers/', data=json.dumps(self.waiver_data), + r = client.post('/api/v1.0/waivers/', data=json.dumps(WAIVER_DATA), content_type='application/json', headers=headers) assert r.status_code == 201 assert r.headers.get('WWW-Authenticate') == \ @@ -72,7 +82,7 @@ def test_authorized(self, client, monkeypatch): def test_invalid_token(self, client, monkeypatch): monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab') headers = {'Authorization': 'Negotiate INVALID'} - r = client.post('/api/v1.0/waivers/', data=json.dumps(self.waiver_data), + r = client.post('/api/v1.0/waivers/', data=json.dumps(WAIVER_DATA), content_type='application/json', headers=headers) assert r.status_code == 401 assert r.json == {"message": "Invalid authentication token"} @@ -146,6 +156,79 @@ def test_no_warning_banner( assert "alert" not in r.text assert r.status_code == 200 + def test_new_waiver_banner( + self, + oidc_token, + session, + client, + ): + headers = {'Authorization': 'Bearer foobar'} + r = client.get('/api/v1.0/waivers/new?new_waiver_id=123', headers=headers) + banner = ( + '' + ) + assert banner in r.text + assert r.status_code == 200 + + def test_create_new_waiver( + self, + verify_authorization, + permissions, + oidc_token, + session, + client, + ): + verify_authorization.return_value = True + permissions.return_value = [{"testcases": ["a.b.c"], "groups": []}] + headers = {'Authorization': 'Bearer foobar'} + url = f'/api/v1.0/waivers/create?{WAIVER_PARAMS}' + r = client.get( + url, + headers=headers, + follow_redirects=True, + ) + assert 'New waiver created.' in r.text + assert r.status_code == 200 + assert r.request.base_url.endswith('/api/v1.0/waivers/new') + expected_args = { + k: v + for k, v in WAIVER_DATA.items() + if isinstance(v, str) + } + expected_args['new_waiver_id'] = '1' + assert dict(r.request.args) == expected_args + + def test_create_new_waiver_unauthorized( + self, + verify_authorization, + permissions, + oidc_token, + session, + client, + ): + verify_authorization.side_effect = Unauthorized("Unauthorized") + permissions.return_value = [{"testcases": ["a.b.c"], "groups": []}] + headers = {'Authorization': 'Bearer foobar'} + url = f'/api/v1.0/waivers/create?{WAIVER_PARAMS}' + r = client.get( + url, + headers=headers, + follow_redirects=True, + ) + assert '401 Unauthorized: Unauthorized' in r.text + assert 'New waiver created.' not in r.text + assert r.status_code == 200 + assert r.request.base_url.endswith('/api/v1.0/waivers/new') + expected_args = { + k: v + for k, v in WAIVER_DATA.items() + if isinstance(v, str) + } + expected_args['error'] = mock.ANY + assert dict(r.request.args) == expected_args + @pytest.mark.usefixtures('enable_ssl') class TestSSLAuthentication(object): @@ -176,7 +259,7 @@ def test_good_ssl_cert(self): class TestKerberosWithFallbackAuthentication(TestGSSAPIAuthentication): def test_unauthorized(self, client, monkeypatch): monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab') - r = client.post('/api/v1.0/waivers/', data=json.dumps(self.waiver_data), + r = client.post('/api/v1.0/waivers/', data=json.dumps(WAIVER_DATA), content_type='application/json') assert r.status_code == 401 diff --git a/waiverdb/api_v1.py b/waiverdb/api_v1.py index 9f88321..22956e3 100644 --- a/waiverdb/api_v1.py +++ b/waiverdb/api_v1.py @@ -8,6 +8,7 @@ Response, current_app, escape, + redirect, render_template, request, url_for, @@ -107,6 +108,16 @@ def _verify_authorization(user, testcase): return verify_authorization(user, testcase, permissions(), ldap_host, ldap_searches) +def _authorization_warning_from_exception(e: Unauthorized, testcase: str): + permissions_url = url_for('api_v1.permissionsresource', testcase=testcase) + return ( + f"{escape(str(e))}
" + f"See who has permission to" + f" waive {escape(testcase)} test case." + "" + ) + + def _authorization_warning(request): testcase = request.args.get("testcase") if testcase: @@ -114,13 +125,7 @@ def _authorization_warning(request): try: _verify_authorization(user, testcase) except Unauthorized as e: - permissions_url = url_for('api_v1.permissionsresource', testcase=testcase) - return ( - f"{escape(str(e))}
" - f"See who has permission to" - f" waive {escape(testcase)} test case." - "" - ) + return _authorization_warning_from_exception(e, testcase) return None @@ -378,19 +383,46 @@ def get(self): :statuscode 200: The HTML with the form is returned. """ - warning = _authorization_warning(request) - html = render_template('new_waiver.html', warning=warning, request_args=request.args) + warning = request.args.get("error") or _authorization_warning(request) + new_waiver_id = request.args.get("new_waiver_id") + new_waiver_url = None + if new_waiver_id is not None: + new_waiver_url = url_for('api_v1.waiverresource', waiver_id=new_waiver_id) + html = render_template( + 'new_waiver.html', + warning=warning, + error=request.args.get("error"), + new_waiver_url=new_waiver_url, + request_args=request.args, + ) return Response(html, mimetype='text/html') + +class WaiversCreateResource(WaiversResource): @oidc.require_login - @validate(form=CreateWaiver) - @marshal_with(waiver_fields) - def post(self): + @validate() + def get(self, query: CreateWaiver): user = oidc.user_getfield(current_app.config["OIDC_USERNAME_FIELD"]) - result = self._create_waiver(CreateWaiver.parse_obj(request.form), user) + try: + result = self._create_waiver(query, user) + except Unauthorized as e: + error = _authorization_warning_from_exception(e, query.testcase) + url = url_for( + "api_v1.waiversnewresource", + error=error, + **request.args, + ) + return redirect(url) + db.session.add(result) db.session.commit() - return result, 201 + + url = url_for( + "api_v1.waiversnewresource", + new_waiver_id=result.id, + **request.args, + ) + return redirect(url) class WaiverResource(Resource): @@ -708,6 +740,7 @@ def get(self): # set up the Api resource routing here api.add_resource(WaiversResource, '/waivers/') api.add_resource(WaiversNewResource, '/waivers/new') +api.add_resource(WaiversCreateResource, '/waivers/create') api.add_resource(WaiverResource, '/waivers/') api.add_resource(FilteredWaiversResource, '/waivers/+filtered') api.add_resource(GetWaiversBySubjectsAndTestcases, '/waivers/+by-subjects-and-testcases') diff --git a/waiverdb/templates/new_waiver.html b/waiverdb/templates/new_waiver.html index 8017770..f39f389 100644 --- a/waiverdb/templates/new_waiver.html +++ b/waiverdb/templates/new_waiver.html @@ -8,7 +8,10 @@ {% if warning %} {% endif %} -
+ {% if new_waiver_url %} + + {% endif %} +