From 3001c2dde1f87096fcecf3c1003783d5cba805d5 Mon Sep 17 00:00:00 2001 From: Alastair Porter Date: Tue, 7 Jun 2022 18:45:43 +0200 Subject: [PATCH 1/2] Provide a user-friendly error if oauth params are incorrect An invalid client id, mismatching redirect uri, or bad scope resulted in an unhandled HTTP500 error, as the exception raised is only handled in the API app, not the website app. Catch and re-raise this exception so that it is properly handled. Update db.oauth_client.create to actually return a dictionary as documented, this is needed for the test. --- critiquebrainz/db/oauth_client.py | 4 +- critiquebrainz/frontend/testing.py | 2 + critiquebrainz/frontend/views/oauth.py | 22 ++++-- .../frontend/views/test/test_oauth.py | 77 +++++++++++++++++++ 4 files changed, 97 insertions(+), 8 deletions(-) create mode 100644 critiquebrainz/frontend/views/test/test_oauth.py diff --git a/critiquebrainz/db/oauth_client.py b/critiquebrainz/db/oauth_client.py index 35db176a6..a7d0e8f04 100644 --- a/critiquebrainz/db/oauth_client.py +++ b/critiquebrainz/db/oauth_client.py @@ -30,11 +30,12 @@ def create(*, user_id, name, desc, website, redirect_uri): } """ with db.engine.connect() as connection: - connection.execute(sqlalchemy.text(""" + row = connection.execute(sqlalchemy.text(""" INSERT INTO oauth_client (client_id, client_secret, redirect_uri, user_id, name, "desc", website) VALUES (:client_id, :client_secret, :redirect_uri, :user_id, :name, :desc, :website) + RETURNING client_id, client_secret, redirect_uri, user_id, name, "desc", website """), { "client_id": generate_string(CLIENT_ID_LENGTH), "client_secret": generate_string(CLIENT_SECRET_LENGTH), @@ -44,6 +45,7 @@ def create(*, user_id, name, desc, website, redirect_uri): "website": website, "desc": desc, }) + return dict(row.fetchone()) def update(*, client_id, name=None, desc=None, website=None, redirect_uri=None): diff --git a/critiquebrainz/frontend/testing.py b/critiquebrainz/frontend/testing.py index 951fa0fb4..a91a9688c 100644 --- a/critiquebrainz/frontend/testing.py +++ b/critiquebrainz/frontend/testing.py @@ -1,4 +1,5 @@ import os +from critiquebrainz.ws.oauth import oauth from flask_testing import TestCase @@ -10,6 +11,7 @@ class FrontendTestCase(TestCase): def create_app(self): app = create_app(config_path=os.path.join(os.path.dirname(os.path.realpath(__file__)), '..', '..', 'test_config.py')) + oauth.init_app(app) return app def setUp(self): diff --git a/critiquebrainz/frontend/views/oauth.py b/critiquebrainz/frontend/views/oauth.py index b33c705ef..c08e06057 100644 --- a/critiquebrainz/frontend/views/oauth.py +++ b/critiquebrainz/frontend/views/oauth.py @@ -1,4 +1,6 @@ from flask import Blueprint, render_template, redirect, request +from werkzeug.exceptions import BadRequest +from critiquebrainz.ws.oauth.exceptions import OAuthError from flask_login import login_required, current_user import critiquebrainz.db.oauth_client as db_oauth_client @@ -19,12 +21,18 @@ def authorize_prompt(): state = request.args.get('state') if request.method == 'GET': # Client requests access - oauth.validate_authorization_request(client_id, response_type, redirect_uri, scope) - client = db_oauth_client.get_client(client_id) - return render_template('oauth/prompt.html', client=client, scope=scope, - cancel_url=build_url(redirect_uri, dict(error='access_denied'))) + try: + oauth.validate_authorization_request(client_id, response_type, redirect_uri, scope) + client = db_oauth_client.get_client(client_id) + return render_template('oauth/prompt.html', client=client, scope=scope, + cancel_url=build_url(redirect_uri, dict(error='access_denied'))) + except OAuthError as e: + raise BadRequest(e.desc) if request.method == 'POST': # User grants access to the client - oauth.validate_authorization_request(client_id, response_type, redirect_uri, scope) - code = oauth.generate_grant(client_id, current_user.id, redirect_uri, scope) - return redirect(build_url(redirect_uri, dict(code=code, state=state))) + try: + oauth.validate_authorization_request(client_id, response_type, redirect_uri, scope) + code = oauth.generate_grant(client_id, current_user.id, redirect_uri, scope) + return redirect(build_url(redirect_uri, dict(code=code, state=state))) + except OAuthError as e: + raise BadRequest(e.desc) \ No newline at end of file diff --git a/critiquebrainz/frontend/views/test/test_oauth.py b/critiquebrainz/frontend/views/test/test_oauth.py new file mode 100644 index 000000000..998d47d0f --- /dev/null +++ b/critiquebrainz/frontend/views/test/test_oauth.py @@ -0,0 +1,77 @@ +from flask import url_for +import critiquebrainz.db.oauth_client as db_oauth_client +import critiquebrainz.db.oauth_grant as db_oauth_grant +import critiquebrainz.db.users as db_users +from critiquebrainz.frontend.testing import FrontendTestCase + +from urllib.parse import urlparse, parse_qs + + +class OauthTestCase(FrontendTestCase): + def setUp(self): + from critiquebrainz.db.user import User + self.user = User(db_users.get_or_create(2, "9371e5c7-5995-4471-a5a9-33481f897f9c", new_user_data={ + "display_name": u"User", + })) + self.oauthclient = db_oauth_client.create( + user_id=self.user.id, + name='An oauth app', + desc='This is a great client', + website='https://example.com', + redirect_uri='https://example.com/redirect' + ) + + + def test_invalid_clientid(self): + self.temporary_login(self.user) + response = self.client.get(url_for('oauth.authorize_prompt', response_type='code', client_id='not-an-id', redirect_uri='x', scope='x', state='x')) + assert response.status_code == 400 + assert "400 Bad Request: Client authentication failed." in response.text + + def test_invalid_redirect_uri(self): + self.temporary_login(self.user) + client_id = self.oauthclient["client_id"] + response = self.client.get(url_for('oauth.authorize_prompt', response_type='code', client_id=client_id, redirect_uri='x', scope='x', state='x')) + assert response.status_code == 400 + assert "400 Bad Request: Invalid redirect uri." in response.text + + def test_invalid_scope(self): + self.temporary_login(self.user) + client_id = self.oauthclient["client_id"] + redirect_uri = self.oauthclient["redirect_uri"] + response = self.client.get(url_for('oauth.authorize_prompt', response_type='code', client_id=client_id, redirect_uri=redirect_uri, scope='x', state='x')) + assert response.status_code == 400 + assert "400 Bad Request: The requested scope is invalid, unknown, or malformed." in response.text + + def test_valid_oauth_request(self): + self.temporary_login(self.user) + client_id = self.oauthclient["client_id"] + redirect_uri = self.oauthclient["redirect_uri"] + response = self.client.get(url_for('oauth.authorize_prompt', response_type='code', client_id=client_id, redirect_uri=redirect_uri, scope='review', state='x')) + assert response.status_code == 200 + assert "Do you want to give access to your account to An oauth app?" in response.text + + def test_approve_invalid_parameter(self): + """Same endpoint, but a POST with an invalid client id""" + self.temporary_login(self.user) + response = self.client.post(url_for('oauth.authorize_prompt', response_type='code', client_id='x', redirect_uri='y', scope='review', state='x')) + assert response.status_code == 400 + assert "400 Bad Request: Client authentication failed." in response.text + + def test_approve_application(self): + """A POST to approve an oauth authorization""" + self.temporary_login(self.user) + client_id = self.oauthclient["client_id"] + redirect_uri = self.oauthclient["redirect_uri"] + response = self.client.post(url_for('oauth.authorize_prompt', response_type='code', client_id=client_id, redirect_uri=redirect_uri, scope='review', state='x')) + assert response.status_code == 302 + # This is the redirect URL that we set in the oauth client + assert response.location.startswith('https://example.com/redirect?code=') + + redirect_location = urlparse(response.location) + query = parse_qs(redirect_location.query) + assert query['state'] == ['x'] + code = query['code'][0] + + grants = db_oauth_grant.list_grants(client_id=client_id, code=code) + assert len(grants) == 1 From 26547229c622cf0a4442440a4524d3a8240ff46b Mon Sep 17 00:00:00 2001 From: Alastair Porter Date: Tue, 7 Jun 2022 18:47:55 +0200 Subject: [PATCH 2/2] Don't run code coverage if tests fail --- pytest.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pytest.ini b/pytest.ini index f4f0304f1..d07376c90 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,3 +1,3 @@ [pytest] testpaths = critiquebrainz -addopts = --cov=critiquebrainz +addopts = --cov=critiquebrainz --no-cov-on-fail