From aa96293d1153827ca6939876c737c9c7b703cee7 Mon Sep 17 00:00:00 2001 From: Daniel Serkowski Date: Tue, 13 Aug 2019 13:45:45 +0200 Subject: [PATCH 1/9] OLIMS-6504: Added oauth-init endpoint for auto-approve mechanism --- superset_patchup/oauth.py | 40 +++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/superset_patchup/oauth.py b/superset_patchup/oauth.py index 8cd1a17..2178409 100644 --- a/superset_patchup/oauth.py +++ b/superset_patchup/oauth.py @@ -2,7 +2,7 @@ import logging import re -from flask import abort, flash, g, redirect, request, session, url_for +from flask import abort, flash, g, redirect, request, session, url_for, jsonify, make_response from flask_appbuilder._compat import as_unicode from flask_appbuilder.security.sqla import models as ab_models @@ -49,11 +49,7 @@ def login(self, provider=None, register=None): appbuilder=self.appbuilder, ) logging.debug(f"Going to call authorize for: {provider}") - state = jwt.encode( - request.args.to_dict(flat=False), - self.appbuilder.app.config["SECRET_KEY"], - algorithm="HS256", - ) + state = self.generateState() try: if register: logging.debug("Login to Register") @@ -80,6 +76,32 @@ def login(self, provider=None, register=None): flash(as_unicode(self.invalid_login_message), "warning") return redirect(redirect_url) + @expose("/oauth-init/") + def login_init(self, provider=None): + logging.debug(f"Provider: {provider}") + + if g.user is not None and g.user.is_authenticated: + logging.debug(f"Provider {provider} is already authenticated by {g.user}") + return make_response(jsonify( + isAuthorized=True + )) + + redirect_url = request.args.get("redirect_url") + if not redirect_url or not is_safe_url(redirect_url): + logging.debug(f"The arg redirect_url not found or not safe") + return abort(400) + + logging.debug(f"Initialization of authorization process for: {provider}") + + # libraries assume that 'redirect_url' should be available in the session + session['%s_oauthredir' % provider] = redirect_url + + state = self.generateState() + return make_response(jsonify( + isAuthorized=False, + state=state + )) + @expose("/oauth-authorized/") # pylint: disable=too-many-branches # pylint: disable=logging-fstring-interpolation @@ -141,6 +163,12 @@ def oauth_authorized(self, provider): return redirect(self.appbuilder.get_url_for_index) + def generateState(self): + return jwt.encode( + request.args.to_dict(flat=False), + self.appbuilder.app.config["SECRET_KEY"], + algorithm="HS256", + ) class CustomSecurityManager(SupersetSecurityManager): """Custom Security Manager Class""" From 166c8d02f1f13ccc5469946f4c08bec508b7d70b Mon Sep 17 00:00:00 2001 From: Daniel Serkowski Date: Wed, 14 Aug 2019 17:33:32 +0200 Subject: [PATCH 2/9] OLMIS-6504: Added docstrings and tests --- superset_patchup/oauth.py | 8 ++-- tests/oauth/test_oauth.py | 85 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 89 insertions(+), 4 deletions(-) diff --git a/superset_patchup/oauth.py b/superset_patchup/oauth.py index 2178409..0a6643e 100644 --- a/superset_patchup/oauth.py +++ b/superset_patchup/oauth.py @@ -49,7 +49,7 @@ def login(self, provider=None, register=None): appbuilder=self.appbuilder, ) logging.debug(f"Going to call authorize for: {provider}") - state = self.generateState() + state = self.generate_state() try: if register: logging.debug("Login to Register") @@ -78,6 +78,7 @@ def login(self, provider=None, register=None): @expose("/oauth-init/") def login_init(self, provider=None): + """Checks authorization and if a user has not authenticated, inits the sign-in process and returns a state""" logging.debug(f"Provider: {provider}") if g.user is not None and g.user.is_authenticated: @@ -96,7 +97,7 @@ def login_init(self, provider=None): # libraries assume that 'redirect_url' should be available in the session session['%s_oauthredir' % provider] = redirect_url - state = self.generateState() + state = self.generate_state() return make_response(jsonify( isAuthorized=False, state=state @@ -163,7 +164,8 @@ def oauth_authorized(self, provider): return redirect(self.appbuilder.get_url_for_index) - def generateState(self): + def generate_state(self): + """Generates a state which is required during the OAuth sign-in process""" return jwt.encode( request.args.to_dict(flat=False), self.appbuilder.app.config["SECRET_KEY"], diff --git a/tests/oauth/test_oauth.py b/tests/oauth/test_oauth.py index f427894..5da1039 100644 --- a/tests/oauth/test_oauth.py +++ b/tests/oauth/test_oauth.py @@ -1,7 +1,7 @@ """ This module tests oauth """ -from unittest.mock import MagicMock, patch, call +from unittest.mock import MagicMock, patch, call, PropertyMock from superset import app @@ -340,3 +340,86 @@ def test_is_valid_provider_is_called_for_opendata(self, function_mock): assert call("opensrp", "OpenSRP") in function_mock.call_args_list csm.oauth_user_info(provider="OPENLMIS") assert call("OPENLMIS", "openlmis") in function_mock.call_args_list + + @patch("superset_patchup.oauth.jwt") + @patch("superset_patchup.oauth.jsonify") + @patch("superset_patchup.oauth.make_response") + @patch("superset_patchup.oauth.g") + @patch("superset_patchup.oauth.is_safe_url") + @patch("superset_patchup.oauth.request.args.get") + @patch("superset_patchup.oauth.request") + def test_init_login_result_for_not_authorized_user( + self, + mock_request, + mock_redirect_arg, + mock_safe_url, + mock_g, + mock_make_response, + mock_jsonify, + mock_jwt + ): + """ + Test that checks if the correct response for a not authorized user is returned. + """ + oauth_view = AuthOAuthView() + oauth_view.appbuilder = MagicMock() + + provider = "OPENLMIS" + redirect_url = "/superset/dashboard/3" + state = '12345' + + with patch('superset_patchup.oauth.g.user.is_authenticated', False): + with patch("superset_patchup.oauth.session", dict()) as session: + mock_redirect_arg.return_value = redirect_url + mock_jwt.encode.return_value = state + + oauth_view.login_init(provider=provider) + + mock_make_response.assert_called() + assert call(isAuthorized=False, state=state) in mock_jsonify.call_args_list + assert session.get('%s_oauthredir' % provider) == redirect_url + + @patch("superset_patchup.oauth.jsonify") + @patch("superset_patchup.oauth.make_response") + @patch("superset_patchup.oauth.g") + def test_init_login_result_for_already_authorized_user( + self, + mock_g, + mock_make_response, + mock_jsonify + ): + """ + Test that checks if the correct response for an already authorized user is returned. + """ + oauth_view = AuthOAuthView() + oauth_view.appbuilder = MagicMock() + provider = "OPENLMIS" + + with patch('superset_patchup.oauth.g.user.is_authenticated', True): + with patch("superset_patchup.oauth.session", dict()) as session: + + oauth_view.login_init(provider=provider) + + mock_make_response.assert_called() + assert call(isAuthorized=True) in mock_jsonify.call_args_list + assert (('%s_oauthredir' % provider) in session) is False + + @patch("superset_patchup.oauth.request") + def test_generate_state_result( + self, + mock_request, + ): + """ + Test that checks if a valid state is returned. + """ + oauth_view = AuthOAuthView() + oauth_view.appbuilder = MagicMock() + app_config = dict(SECRET_KEY='secret_key') + request_args = dict(dummy_parameter='dummy_parameter_value') + + type(oauth_view.appbuilder.app).config = PropertyMock(return_value=app_config) + mock_request.args.to_dict.return_value = request_args + + state = oauth_view.generate_state() + + assert len(state) > 0 From cbe51c3eaba0dd1c5793f7f7576e77ef5a3b9121 Mon Sep 17 00:00:00 2001 From: Kelvin Jayanoris Date: Mon, 19 Aug 2019 13:02:35 +0300 Subject: [PATCH 3/9] Add pyjwt to requirements --- requirements/dev.in | 1 + requirements/dev.txt | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/requirements/dev.in b/requirements/dev.in index 903aa9a..584fbfb 100644 --- a/requirements/dev.in +++ b/requirements/dev.in @@ -20,3 +20,4 @@ urllib3>=1.24.2 sqlalchemy>=1.3.0 jinja2>=2.10.1 parso>=0.5.0 +pyjwt diff --git a/requirements/dev.txt b/requirements/dev.txt index cc1e587..86e9d8b 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -2,14 +2,13 @@ # This file is autogenerated by pip-compile # To update, run: # -# pip-compile --output-file requirements/dev.txt requirements/dev.in +# pip-compile requirements/dev.in # alembic==1.0.10 # via flask-migrate amqp==2.4.2 # via kombu apispec[yaml]==1.3.3 # via flask-appbuilder appdirs==1.4.3 # via black -appnope==0.1.0 # via ipython asn1crypto==0.24.0 # via cryptography astroid==2.2.5 # via pylint, pylint-flask atomicwrites==1.3.0 # via pytest @@ -103,7 +102,7 @@ pydruid==0.5.2 # via superset pyflakes==2.1.1 # via flake8 pygments==2.3.1 # via ipython pyhive==0.6.1 # via superset -pyjwt==1.7.1 # via flask-appbuilder, flask-jwt-extended +pyjwt==1.7.1 pylint-flask==0.6 pylint-plugin-utils==0.5 # via pylint-flask pylint==2.3.1 @@ -147,3 +146,6 @@ wtforms==2.2.1 # via flask-wtf xlrd==1.2.0 # via tabulator yapf==0.27.0 zipp==0.4.0 # via importlib-metadata + +# The following packages are considered to be unsafe in a requirements file: +# setuptools==41.1.0 # via ipdb, ipython, jsonschema, markdown, pytest, tox From 0c7fed518dbd0e50043aebee16fdd5e32cb80ce5 Mon Sep 17 00:00:00 2001 From: Daniel Serkowski Date: Mon, 19 Aug 2019 13:49:44 +0200 Subject: [PATCH 4/9] OLMIS-6504: Fixed CI errors --- superset_patchup/oauth.py | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/superset_patchup/oauth.py b/superset_patchup/oauth.py index 0a6643e..9939ba9 100644 --- a/superset_patchup/oauth.py +++ b/superset_patchup/oauth.py @@ -2,7 +2,8 @@ import logging import re -from flask import abort, flash, g, redirect, request, session, url_for, jsonify, make_response +from flask import abort, flash, g, redirect, request, session, url_for, \ + jsonify, make_response from flask_appbuilder._compat import as_unicode from flask_appbuilder.security.sqla import models as ab_models @@ -78,23 +79,29 @@ def login(self, provider=None, register=None): @expose("/oauth-init/") def login_init(self, provider=None): - """Checks authorization and if a user has not authenticated, inits the sign-in process and returns a state""" - logging.debug(f"Provider: {provider}") + """ + Checks authorization and if a user is not authorized, + inits the sign-in process and returns a state + """ + logging.debug("Provider: %s", provider) if g.user is not None and g.user.is_authenticated: - logging.debug(f"Provider {provider} is already authenticated by {g.user}") + logging.debug("Provider %s is already authorized by %s", + provider, g.user) return make_response(jsonify( isAuthorized=True )) redirect_url = request.args.get("redirect_url") if not redirect_url or not is_safe_url(redirect_url): - logging.debug(f"The arg redirect_url not found or not safe") + logging.debug("The arg redirect_url not found or not safe") return abort(400) - logging.debug(f"Initialization of authorization process for: {provider}") + logging.debug("Initialization of authorization process for: %s", + provider) - # libraries assume that 'redirect_url' should be available in the session + # libraries assume that + # 'redirect_url' should be available in the session session['%s_oauthredir' % provider] = redirect_url state = self.generate_state() @@ -165,13 +172,16 @@ def oauth_authorized(self, provider): return redirect(self.appbuilder.get_url_for_index) def generate_state(self): - """Generates a state which is required during the OAuth sign-in process""" + """ + Generates a state which is required during the OAuth sign-in process + """ return jwt.encode( request.args.to_dict(flat=False), self.appbuilder.app.config["SECRET_KEY"], algorithm="HS256", ) + class CustomSecurityManager(SupersetSecurityManager): """Custom Security Manager Class""" From a96fe6d3c77452b7a38f9f25084a1ccd8225de73 Mon Sep 17 00:00:00 2001 From: Daniel Serkowski Date: Mon, 19 Aug 2019 13:50:16 +0200 Subject: [PATCH 5/9] OLMIS-6504: Extended README --- README.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/README.md b/README.md index caa24ad..868364e 100644 --- a/README.md +++ b/README.md @@ -116,6 +116,13 @@ Ketchup's CustomSecurityManager class includes a custom `oauth_user_info` method - `openlmis` - `OpenSRP` +### Custom oAuth init endpoint + +One of the ideas of integration Superset with an external service (eg. OpenLMIS) is to allow run the application and sign-in directly from the external application's API. To simplify the whole process, Ketchup provides the endpoint `/oauth-init/`. Its backend functionality does the same what the endpoint `/login/` does, however it doesn't redirect a client. Instead of it, it returns information in the JSON form which contains the following fields: + +* `isAuthorized` - the flag which provides information about user's authorization in the app +* `state` - oAuth state which is required during the oAuth sign-in process. It is provided only if `isAuthorized` is false + #### PATCHUP_EMAIL_BASE In cases where an oAuth provider does not provide an email address for its users, Superset's oAuth process might fail. To remedy this, Superset-patchup you can set the `PATCHUP_EMAIL_BASE` variable in `superset_config.py`. From fe143d140056dd52c54ddb44974bf42b1d5c0988 Mon Sep 17 00:00:00 2001 From: Kelvin Jayanoris Date: Mon, 19 Aug 2019 15:01:22 +0300 Subject: [PATCH 6/9] Cleanup docs --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 868364e..694a0ac 100644 --- a/README.md +++ b/README.md @@ -118,10 +118,10 @@ Ketchup's CustomSecurityManager class includes a custom `oauth_user_info` method ### Custom oAuth init endpoint -One of the ideas of integration Superset with an external service (eg. OpenLMIS) is to allow run the application and sign-in directly from the external application's API. To simplify the whole process, Ketchup provides the endpoint `/oauth-init/`. Its backend functionality does the same what the endpoint `/login/` does, however it doesn't redirect a client. Instead of it, it returns information in the JSON form which contains the following fields: +One of the ideas of integration Superset with an external service (eg. OpenLMIS) is to allow running the application and sign-in directly from the external application's API. To simplify the whole process, Ketchup provides the endpoint `/oauth-init/`. It is a backend functionality that essentially does what the endpoint `/login/` does, however it doesn't redirect a client. Instead of it, it returns information in the JSON form which contains the following fields: -* `isAuthorized` - the flag which provides information about user's authorization in the app -* `state` - oAuth state which is required during the oAuth sign-in process. It is provided only if `isAuthorized` is false +- `isAuthorized` - the flag which provides information about user's authorization in the app +- `state` - oAuth state which is required during the oAuth sign-in process. It is provided only if `isAuthorized` is false #### PATCHUP_EMAIL_BASE From a763e4dd54c9984e3b442de8d6fe73bf23b8a4a4 Mon Sep 17 00:00:00 2001 From: Kelvin Jayanoris Date: Mon, 19 Aug 2019 15:02:53 +0300 Subject: [PATCH 7/9] Add `tests/.superset/` to gitignore --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 7696fb1..3d301c4 100644 --- a/.gitignore +++ b/.gitignore @@ -103,5 +103,8 @@ venv.bak/ # mypy .mypy_cache/ +# others +tests/.superset/ + # vscode .vscode/ \ No newline at end of file From ec7ae484824ac0b63050b625287ea0b180fbbadb Mon Sep 17 00:00:00 2001 From: Kelvin Jayanoris Date: Mon, 19 Aug 2019 15:07:09 +0300 Subject: [PATCH 8/9] Add pydocstyle to requirements --- requirements/dev.in | 1 + requirements/dev.txt | 2 ++ 2 files changed, 3 insertions(+) diff --git a/requirements/dev.in b/requirements/dev.in index 584fbfb..f176422 100644 --- a/requirements/dev.in +++ b/requirements/dev.in @@ -7,6 +7,7 @@ pylint-flask tox flake8 pep8 +pydocstyle coverage yapf isort diff --git a/requirements/dev.txt b/requirements/dev.txt index 86e9d8b..0651241 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -98,6 +98,7 @@ ptyprocess==0.6.0 # via pexpect py==1.8.0 # via pytest, tox pycodestyle==2.5.0 # via flake8 pycparser==2.19 # via cffi +pydocstyle==4.0.1 pydruid==0.5.2 # via superset pyflakes==2.1.1 # via flake8 pygments==2.3.1 # via ipython @@ -120,6 +121,7 @@ s3transfer==0.1.13 # via boto3 sasl==0.2.1 # via thrift-sasl simplejson==3.16.0 # via superset six==1.12.0 # via astroid, bleach, cryptography, flask-jwt-extended, isodate, jsonlines, jsonschema, linear-tsv, pathlib2, pip-tools, polyline, prison, prompt-toolkit, pydruid, pyrsistent, pytest, python-dateutil, sasl, sqlalchemy-utils, tableschema, tabulator, thrift, tox, traitlets +snowballstemmer==1.9.0 # via pydocstyle sqlalchemy-utils==0.33.11 # via superset sqlalchemy==1.3.3 sqlparse==0.3.0 # via superset From 390905445fd3af69356060ad4052c52453ef8844 Mon Sep 17 00:00:00 2001 From: Kelvin Jayanoris Date: Mon, 19 Aug 2019 15:08:43 +0300 Subject: [PATCH 9/9] =?UTF-8?q?=E2=86=91=20bump=20to=20version=200.1.5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- superset_patchup/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset_patchup/__init__.py b/superset_patchup/__init__.py index 5bf8d1d..78c44ca 100644 --- a/superset_patchup/__init__.py +++ b/superset_patchup/__init__.py @@ -1,5 +1,5 @@ """ Main init file for superset_patchup """ -VERSION = (0, 1, 4) +VERSION = (0, 1, 5) __version__ = ".".join(str(v) for v in VERSION)