flask and python3 conversion #42

FedericOldani · 2021-06-10T16:13:29Z

Hi there!
Since few people asked for python 3 version of this useful ckan extension (issue #39), I implemented it. I converted the code to be compatible with ckan 2.9 which uses python3 and Flask (instead of Pylons). It is now no more compatible with Pylons.
I did a dirty job of conversion, the best way would be re-implement the extension from zero but this is a starting point.

aitormagan

Hi! First of all I want to thank you for this contribution. However, I'm afraid I cannot accept this PR as there are some changes which are required to be done before merging. Some of them are related with the comments I left in the code, but there is another important one: tests need to be adapted to the new code.

I'll be glad to review the PR again when you fix these issues.

aitormagan · 2021-06-11T15:32:11Z

INSTALL.md

@@ -46,7 +46,7 @@ ckan.oauth2.authorization_header = OAUTH2_HEADER
 > ckan.oauth2.authorization_header = Authorization
 > ```
 >
-> And this is an example for using Google as OAuth2 provider:
+> And this is an theme for using Google as OAuth2 provider:


There are a lot of places where the word example is replaced by theme. Why?

aitormagan · 2021-06-11T15:33:58Z

ckanext/oauth2/oauth2.py

@@ -19,11 +19,11 @@
 # along with OAuth2 CKAN Extension.  If not, see <http://www.gnu.org/licenses/>.


-from __future__ import unicode_literals
+# from __future__ import unicode_literals


Instead of commenting, just drop the line. This happens a lot of times

aitormagan · 2021-06-11T15:34:25Z

ckanext/oauth2/oauth2.py

+            # log.debug(f'self.token_endpoint: {self.token_endpoint}')
+            # log.debug(f'headers: {headers}')
+            # log.debug(f'authorization_response: {toolkit.request.url}')
+            # log.debug(f'client_secret: {self.client_secret}')


Are these lines required?

aitormagan · 2021-06-11T15:34:40Z

ckanext/oauth2/oauth2.py

+        # toolkit.response.status = 302
+        # toolkit.response.location = came_from
+        # return toolkit.redirect_to(came_from)
+        # log.debug(f'come from: {came_from}')
+        # toolkit.response.status = 302
+        # toolkit.response.location = came_from


Remove if not required

aitormagan · 2021-06-11T15:35:49Z

ckanext/oauth2/plugin.py

+# def _get_previous_page(default_page):
+#     if 'came_from' not in toolkit.request.params:
+#         came_from_url = toolkit.request.headers.get('Referer', default_page)
+#     else:
+#         came_from_url = toolkit.request.params.get('came_from', default_page)

-    came_from_url_parsed = urlparse(came_from_url)
+#     came_from_url_parsed = urllib.parse(came_from_url)

-    # Avoid redirecting users to external hosts
-    if came_from_url_parsed.netloc != '' and came_from_url_parsed.netloc != toolkit.request.host:
-        came_from_url = default_page
+#     # Avoid redirecting users to external hosts
+#     if came_from_url_parsed.netloc != '' and came_from_url_parsed.netloc != toolkit.request.host:
+#         came_from_url = default_page

-    # When a user is being logged and REFERER == HOME or LOGOUT_PAGE
-    # he/she must be redirected to the dashboard
-    pages = ['/', '/user/logged_out_redirect']
-    if came_from_url_parsed.path in pages:
-        came_from_url = default_page
+#     # When a user is being logged and REFERER == HOME or LOGOUT_PAGE
+#     # he/she must be redirected to the dashboard
+#     pages = ['/', '/user/logged_out_redirect']
+#     if came_from_url_parsed.path in pages:
+#         came_from_url = default_page



If removed, user won't be redirected to the page they came from...

aitormagan · 2021-06-11T15:36:43Z

ckanext/oauth2/plugin.py

+    # def before_map(self, m):
+    #     log.debug('Setting up the redirections to the OAuth2 service')

-        m.connect('/user/login',
-                  controller='ckanext.oauth2.controller:OAuth2Controller',
-                  action='login')
+    #     m.connect('/user/login',
+    #               controller='ckanext.oauth2.controller:OAuth2Controller',
+    #               action='login')

-        # We need to handle petitions received to the Callback URL
-        # since some error can arise and we need to process them
-        m.connect('/oauth2/callback',
-                  controller='ckanext.oauth2.controller:OAuth2Controller',
-                  action='callback')
+    #     # We need to handle petitions received to the Callback URL
+    #     # since some error can arise and we need to process them
+    #     m.connect('/oauth2/callback',
+    #               controller='ckanext.oauth2.controller:OAuth2Controller',
+    #               action='callback')

-        # Redirect the user to the OAuth service register page
-        if self.register_url:
-            m.redirect('/user/register', self.register_url)
+    #     # Redirect the user to the OAuth service register page
+    #     if self.register_url:
+    #         m.redirect('/user/register', self.register_url)

-        # Redirect the user to the OAuth service reset page
-        if self.reset_url:
-            m.redirect('/user/reset', self.reset_url)
+    #     # Redirect the user to the OAuth service reset page
+    #     if self.reset_url:
+    #         m.redirect('/user/reset', self.reset_url)

-        # Redirect the user to the OAuth service reset page
-        if self.edit_url:
-            m.redirect('/user/edit/{user}', self.edit_url)
+    #     # Redirect the user to the OAuth service reset page
+    #     if self.edit_url:
+    #         m.redirect('/user/edit/{user}', self.edit_url)

-        return m
+    #     return m


We still need this to redirect the user to the appropriate site

aitormagan · 2021-06-11T15:37:44Z

ckanext/oauth2/utils.py

@@ -0,0 +1,223 @@
+# -*- coding: utf-8 -*-


Is this file required? I don't see any references to it...

Such file is linked imported by ckanext/oauth2/cli.py, but I think it is a mistake, and both the file and the import should be deleted.

frafra · 2022-02-15T07:07:27Z

It is a pity that such PR is stuck because of minor changes which can be easily applied. I can work on it if needed.

frafra · 2022-02-15T22:04:56Z

ckanext/oauth2/plugin.py

-from urlparse import urlparse
+import urllib.parse
+from ckanext.oauth2.views import get_blueprints
+from ckanext.cloudstorage.cli import get_commands


FedericOldani · 2022-02-16T10:39:05Z

Thank you guys for your interest in this conversion. Actually, the development is gone a little bit further and I solve some issues. Unfortunately, I didn't have time to address the changes requested. Some of them are due to the try&error process I followed to make things work but for sure they can be written better.
Hope to address this by the end of March but I cannot promise that

frafra · 2022-02-16T11:03:15Z

Actually, the development is gone a little bit further and I solve some issues.

Great! :)

Unfortunately, I didn't have time to address the changes requested. Some of them are due to the try&error process I followed to make things work but for sure they can be written better. Hope to address this by the end of March but I cannot promise that

Feel free to share your progress here if you need help.

jeverling · 2022-02-16T20:10:00Z

Hi all, what is the proper way to setup the DB tables with this approach? If I just try to use the extension based on this code, the tables are not created and I get a psycopg2.errors.UndefinedTable exception.

FedericOldani · 2022-02-17T10:17:05Z

I quickly revised some of the comments, but the most important change is in the creation of the DB table during the first run. This was a big bug in my first version. Hope you guys can run it, let me know!

jeverling · 2022-02-17T11:05:18Z

Thanks, I can confirm the table gets created now. 👍

frafra · 2022-04-01T19:30:24Z

It works just fine for me. I guess the review should be updated :)

frafra

We have a mass replacement of example with theme, which should be reverted, various commented and logging lines that can be dropped, and a file which seems to have been committed by mistake.

frafra · 2022-04-07T11:05:40Z

LICENSE.txt

@@ -125,7 +125,7 @@ work) run the object code and to modify the work, including scripts to
 control those activities.  However, it does not include the work's
 System Libraries, or general-purpose tools or generally available free
 programs which are used unmodified in performing those activities but
-which are not part of the work.  For example, Corresponding Source
+which are not part of the work.  For theme, Corresponding Source


LICENSE.txt should not be modified.

frafra · 2022-04-07T11:06:13Z

ckanext/oauth2/controller.py

@@ -18,16 +18,16 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with OAuth2 CKAN Extension.  If not, see <http://www.gnu.org/licenses/>.

-from __future__ import unicode_literals
+# from __future__ import unicode_literals


Shouldn't be dropped entirely?

frafra · 2022-04-07T11:07:03Z

ckanext/oauth2/db.py

@@ -18,10 +18,17 @@
 # along with OAuth2 CKAN Extension.  If not, see <http://www.gnu.org/licenses/>.

 import sqlalchemy as sa
+import ckan.model.meta as meta
+import logging


There is no logging in this file, except a getLogger. I suggest we discard this.

frafra · 2022-04-07T11:07:11Z

ckanext/oauth2/db.py


-UserToken = None
+log = logging.getLogger(__name__)


Not used, can be discarded.

frafra · 2022-04-07T11:07:38Z

ckanext/oauth2/db.py

@@ -47,3 +54,5 @@ def by_user_name(cls, user_name):
        user_token_table.create(checkfirst=True)

        model.meta.mapper(UserToken, user_token_table)
+
+


These empty lines could be avoided.

frafra · 2022-04-07T11:12:04Z

setup.py

        ]
    },
+        # 'nose.plugins': [
+        #     'pylons = pylons.test:PylonsPlugin'
+        # ]


It can be removed entirely.

frafra · 2022-04-07T11:13:21Z

ckanext/oauth2/cli.py

+# -*- coding: utf-8 -*-
+
+import click
+import ckanext.oauth2.utils as utils


ckanext.oauth2.utils is not used here and should be removed.

frafra · 2022-04-07T11:13:53Z

ckanext/oauth2/utils.py

@@ -0,0 +1,223 @@
+# -*- coding: utf-8 -*-


Such file is linked imported by ckanext/oauth2/cli.py, but I think it is a mistake, and both the file and the import should be deleted.

frafra · 2022-04-07T11:14:16Z

ckanext/oauth2/views.py

@@ -0,0 +1,91 @@
+# -*- coding: utf-8 -*-


Not needed for Python 3.

frafra · 2022-04-07T11:14:31Z

ckanext/oauth2/views.py

+        redirect_url = '/' if redirect_url == constants.INITIAL_PAGE else redirect_url
+        response.location = redirect_url
+        helpers.flash_error(error_description)
+        # make_response((content, 302, headers))


It can be dropped.

frafra · 2022-04-07T11:16:23Z

Hi @FedericOldani! I can help with this PR if needed :)

frafra · 2022-04-13T08:55:40Z

ckanext/oauth2/oauth2.py

@@ -38,23 +37,24 @@

 import jwt

-import constants
+from .constants import *


Wildcard import are a bit dangerous and potentially confusing, but they are not a big issue.

frafra · 2022-04-13T08:58:30Z

It looks much better to me :)
There are a couple of changes related to SSL, to force HTTPS I suppose.

frafra

Redirect URL are partially removed. I suggest we keep them or we drop them entirely (conf, variables, tests etc.).

frafra · 2022-04-13T12:03:37Z

ckanext/oauth2/plugin.py

-        if self.edit_url:
-            m.redirect('/user/edit/{user}', self.edit_url)
-
-        return m


Various redirect have been dropped here, but the redirect configuration and test are still here.

frafra · 2022-04-14T12:38:50Z

ckanext/oauth2/views.py

+
+log = logging.getLogger(__name__)
+
+log = logging.getLogger(__name__)


executed twice

frafra · 2022-04-14T12:59:31Z

ckanext/oauth2/plugin.py

-        came_from_url = toolkit.request.params.get('came_from', default_page)
-
-    came_from_url_parsed = urlparse(came_from_url)
+class _OAuth2Plugin(plugins.SingletonPlugin):


Why split the plugin into two?

frafra · 2022-04-14T17:01:23Z

ckanext/oauth2/cli.py

+def oauth2():
+    """Oauth2 management commands.
+    """
+    pass


Is it needed?

simao-silva · 2022-05-27T23:28:03Z

When using this version I get the following error:

AttributeError: 'Request' object has no attribute 'GET'

This is the full traceback:

 Traceback (most recent call last):
   File "/srv/app/src/ckanext-oauth2/ckanext/oauth2/views.py", line 54, in callback
     token = oauth2helper.get_token()
   File "/srv/app/src/ckanext-oauth2/ckanext/oauth2/oauth2.py", line 116, in get_token
     token = oauth.fetch_token(self.token_endpoint,
   File "/usr/lib/python3.8/site-packages/requests_oauthlib/oauth2_session.py", line 244, in fetch_token
     self._client.parse_request_body_response(r.text, scope=self.scope)
   File "/usr/lib/python3.8/site-packages/oauthlib/oauth2/rfc6749/clients/base.py", line 448, in parse_request_body_response
     self.token = parse_token_response(body, scope=scope)
   File "/usr/lib/python3.8/site-packages/oauthlib/oauth2/rfc6749/parameters.py", line 441, in parse_token_response
     validate_token_parameters(params)
   File "/usr/lib/python3.8/site-packages/oauthlib/oauth2/rfc6749/parameters.py", line 448, in validate_token_parameters
     raise_from_error(params.get('error'), params)
   File "/usr/lib/python3.8/site-packages/oauthlib/oauth2/rfc6749/errors.py", line 399, in raise_from_error
     raise cls(**kwargs)
 oauthlib.oauth2.rfc6749.errors.InvalidGrantError: (invalid_grant) Code not valid

frafra · 2022-05-28T20:12:08Z

Do you also have the same problem with the previous version?

simao-silva · 2022-05-29T00:01:55Z

@frafra If by previous version you mean the original 0.7.0 version the answer is no. When using that version I get the error No module named 'oauth2'.
Here is the full traceback:

 Traceback (most recent call last):
   File "/usr/bin/ckan", line 33, in <module>
     sys.exit(load_entry_point('ckan', 'console_scripts', 'ckan')())
   File "/usr/lib/python3.8/site-packages/click/core.py", line 829, in __call__
     return self.main(*args, **kwargs)
   File "/usr/lib/python3.8/site-packages/click/core.py", line 781, in main
     with self.make_context(prog_name, args, **extra) as ctx:
   File "/usr/lib/python3.8/site-packages/click/core.py", line 700, in make_context
     self.parse_args(ctx, args)
   File "/srv/app/src/ckan/ckan/cli/cli.py", line 115, in parse_args
     result = super(ExtendableGroup, self).parse_args(ctx, args)
   File "/usr/lib/python3.8/site-packages/click/core.py", line 1212, in parse_args
     rest = Command.parse_args(self, ctx, args)
   File "/usr/lib/python3.8/site-packages/click/core.py", line 1048, in parse_args
     value, args = param.handle_parse_result(ctx, opts, args)
   File "/usr/lib/python3.8/site-packages/click/core.py", line 1630, in handle_parse_result
     value = invoke_param_callback(self.callback, ctx, self, value)
   File "/usr/lib/python3.8/site-packages/click/core.py", line 123, in invoke_param_callback
     return callback(ctx, param, value)
   File "/srv/app/src/ckan/ckan/cli/cli.py", line 125, in _init_ckan_config
     _add_ctx_object(ctx, value)
   File "/srv/app/src/ckan/ckan/cli/cli.py", line 134, in _add_ctx_object
     ctx.obj = CtxObject(path)
   File "/srv/app/src/ckan/ckan/cli/cli.py", line 56, in __init__
     self.app = make_app(self.config)
   File "/srv/app/src/ckan/ckan/config/middleware/__init__.py", line 56, in make_app
     load_environment(conf)
   File "/srv/app/src/ckan/ckan/config/environment.py", line 123, in load_environment
     p.load_all()
   File "/srv/app/src/ckan/ckan/plugins/core.py", line 165, in load_all
     load(*plugins)
   File "/srv/app/src/ckan/ckan/plugins/core.py", line 179, in load
     service = _get_service(plugin)
   File "/srv/app/src/ckan/ckan/plugins/core.py", line 281, in _get_service
     return plugin.load()(name=plugin_name)
   File "/usr/lib/python3.8/site-packages/pkg_resources/__init__.py", line 2443, in load
     return self.resolve()
   File "/usr/lib/python3.8/site-packages/pkg_resources/__init__.py", line 2449, in resolve
     module = __import__(self.module_name, fromlist=['__name__'], level=0)
   File "/srv/app/src/ckanext-oauth2/ckanext/oauth2/plugin.py", line 24, in <module>
     import oauth2
 ModuleNotFoundError: No module named 'oauth2'

frafra · 2022-05-29T10:44:14Z

@simao-silva that is a missing dependency that you haven't installed. If you cannot test a previous version, then it is hard to determine if you are hitting a regression.

simao-silva · 2022-05-29T14:03:41Z

@frafra I believe you are referring to python-oauth2. Still, if installed, I get the same error but if I install the extension using the code from this PR the error No module named 'oauth2' no longer appears.
This only happens when using Python3. When using Python2, the version 0.7.0 works fine and does not need python-oauth2.

frafra · 2022-07-14T16:44:45Z

@simao-silva try https://github.com/frafra/ckanext-oauth2/tree/flask_conversion_and_ckan_auth
revision 01da047 from branch. It contains a previous snapshot of this PR.

frafra · 2022-07-17T07:08:26Z

@aitormagan any feedback on this PR?

simao-silva · 2022-07-18T20:25:41Z

@simao-silva try https://github.com/frafra/ckanext-oauth2/tree/flask_conversion_and_ckan_auth revision 01da047 from branch. It contains a previous snapshot of this PR.

@frafra that revisions does not forward to the oauth2 endpoint automatically when clicking on login. Still, if I correct that issue, the response from the authentication provider (Keycloak) throws the following error:

 2022-07-18 19:52:44,112 ERROR [ckan.config.middleware.flask_app] 'Request' object has no attribute 'GET'
 Traceback (most recent call last):
   File "/srv/app/src/ckanext-oauth2/ckanext/oauth2/views.py", line 58, in callback
     user_name = oauth2helper.identify(token)
   File "/srv/app/src/ckanext-oauth2/ckanext/oauth2/oauth2.py", line 131, in identify
     access_token = bytes(token['access_token'])
 TypeError: string argument without an encoding
 
 During handling of the above exception, another exception occurred:
 
 Traceback (most recent call last):
   File "/usr/lib/python3.8/site-packages/flask/app.py", line 1949, in full_dispatch_request
     rv = self.dispatch_request()
   File "/usr/lib/python3.8/site-packages/flask/app.py", line 1935, in dispatch_request
     return self.view_functions[rule.endpoint](**req.view_args)
   File "/srv/app/src/ckanext-oauth2/ckanext/oauth2/views.py", line 69, in callback
     error_description = toolkit.request.GET.get('error_description')
   File "/usr/lib/python3.8/site-packages/werkzeug/local.py", line 347, in __getattr__
     return getattr(self._get_current_object(), name)
   File "/usr/lib/python3.8/site-packages/werkzeug/local.py", line 347, in __getattr__
     return getattr(self._get_current_object(), name)
 AttributeError: 'Request' object has no attribute 'GET'

frafra · 2022-08-14T15:49:37Z

@simao-silva I currently use revision 3633240; you could try that one.

frafra · 2022-08-14T15:52:42Z

@aitormagan is unresponsive, and this is critical issue for the project, which seems dead to me.
If nothing changes, we should just maintain a fork elsewhere.
Any thoughts on that?

aitormagan · 2022-08-16T10:57:03Z

I am not unresponsive... I reviewed the code and highlighted things that must be changed before the PR can be merged... In addition, tests are required to be adapted before the PR can be approved.

BR
Aitor

frafra · 2022-10-10T16:00:08Z

@FedericOldani there are some required small changes that need to be made before having your PR being accepted; do you have the time to make them?

flask and python3 conversion

e33ff06

aitormagan requested changes Jun 11, 2021

View reviewed changes

frafra reviewed Feb 15, 2022

View reviewed changes

frafra mentioned this pull request Feb 16, 2022

Flask conversion #46

Closed

federicooldani added 2 commits February 17, 2022 11:09

creation DB table fixed, small improvements

390bdb6

dropped some debug comments

3633240

frafra suggested changes Apr 7, 2022

View reviewed changes

first fix after frafra code review

223175f

frafra reviewed Apr 13, 2022

View reviewed changes

frafra suggested changes Apr 13, 2022

View reviewed changes

frafra reviewed Apr 14, 2022

View reviewed changes

ckanext/oauth2/views.py Outdated

log = logging.getLogger(__name__)

log = logging.getLogger(__name__)

Copy link

frafra Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executed twice

frafra reviewed Apr 14, 2022

View reviewed changes

ckanext/oauth2/cli.py

def oauth2():

"""Oauth2 management commands.

"""

pass

Copy link

frafra Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed?

minor fix

9ff9977

frafra mentioned this pull request Aug 6, 2023

version upgrade for ckan2.9 #45

Open

		@@ -47,3 +54,5 @@ def by_user_name(cls, user_name):
		user_token_table.create(checkfirst=True)

		model.meta.mapper(UserToken, user_token_table)


		log = logging.getLogger(__name__)

		log = logging.getLogger(__name__)

flask and python3 conversion #42

Are you sure you want to change the base?

flask and python3 conversion #42

Conversation

FedericOldani commented Jun 10, 2021

aitormagan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frafra commented Feb 15, 2022

Choose a reason for hiding this comment

FedericOldani commented Feb 16, 2022

frafra commented Feb 16, 2022

jeverling commented Feb 16, 2022

FedericOldani commented Feb 17, 2022

jeverling commented Feb 17, 2022

frafra commented Apr 1, 2022

frafra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frafra commented Apr 7, 2022

Choose a reason for hiding this comment

frafra commented Apr 13, 2022 • edited

frafra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simao-silva commented May 27, 2022

frafra commented May 28, 2022

simao-silva commented May 29, 2022 • edited

frafra commented May 29, 2022

simao-silva commented May 29, 2022 • edited

frafra commented Jul 14, 2022

frafra commented Jul 17, 2022

simao-silva commented Jul 18, 2022

frafra commented Aug 14, 2022

frafra commented Aug 14, 2022

aitormagan commented Aug 16, 2022

frafra commented Oct 10, 2022

aitormagan left a comment •

edited

frafra commented Apr 13, 2022 •

edited

simao-silva commented May 29, 2022 •

edited

simao-silva commented May 29, 2022 •

edited