From e7f87acdeb754280a7b7457217a9734812de4224 Mon Sep 17 00:00:00 2001 From: Brook Elgie Date: Tue, 18 Nov 2014 15:49:25 +0000 Subject: [PATCH 1/7] [#1661] Remove repoze.who OpenID plugin --- ckan/config/middleware.py | 15 -- ckan/config/who.ini | 26 +-- ckan/controllers/user.py | 8 - ckan/lib/app_globals.py | 1 - ckan/lib/authenticator.py | 16 +- ckan/lib/repoze_patch.py | 242 --------------------------- ckan/tests/lib/test_authenticator.py | 55 ------ ckan/tests/test_coding_standards.py | 2 - requirements.in | 1 - requirements.txt | 1 - 10 files changed, 2 insertions(+), 365 deletions(-) delete mode 100644 ckan/lib/repoze_patch.py diff --git a/ckan/config/middleware.py b/ckan/config/middleware.py index 344adf77b2c..13a87f874e1 100644 --- a/ckan/config/middleware.py +++ b/ckan/config/middleware.py @@ -117,21 +117,6 @@ def make_app(conf, full_stack=True, static_files=True, **app_conf): who_parser = WhoConfig(conf['here']) who_parser.parse(open(app_conf['who.config_file'])) - if asbool(config.get('openid_enabled', 'true')): - from repoze.who.plugins.openid.identification import OpenIdIdentificationPlugin - # Monkey patches for repoze.who.openid - # Fixes #1659 - enable log-out when CKAN mounted at non-root URL - from ckan.lib import repoze_patch - OpenIdIdentificationPlugin.identify = repoze_patch.identify - OpenIdIdentificationPlugin.redirect_to_logged_in = repoze_patch.redirect_to_logged_in - OpenIdIdentificationPlugin._redirect_to_loginform = repoze_patch._redirect_to_loginform - OpenIdIdentificationPlugin.challenge = repoze_patch.challenge - - who_parser.identifiers = [i for i in who_parser.identifiers if \ - not isinstance(i, OpenIdIdentificationPlugin)] - who_parser.challengers = [i for i in who_parser.challengers if \ - not isinstance(i, OpenIdIdentificationPlugin)] - app = PluggableAuthenticationMiddleware(app, who_parser.identifiers, who_parser.authenticators, diff --git a/ckan/config/who.ini b/ckan/config/who.ini index 293569878dd..ff0554181bb 100644 --- a/ckan/config/who.ini +++ b/ckan/config/who.ini @@ -17,46 +17,22 @@ charset = utf-8 #use = repoze.who.plugins.basicauth:make_plugin #realm = 'CKAN' -[plugin:openid] -use = repoze.who.plugins.openid:make_identification_plugin -store = file -store_file_path = /tmp/sstore -#openid_field = openid -openid_field = openid_identifier -came_from_field = came_from -error_field = error -session_name = beaker.session -login_form_url = /user/login -login_handler_path = /login_openid -logout_handler_path = /user/logout -# important they go via here after login -logged_in_url = /user/logged_in -logged_out_url = /user/logged_out -rememberer_name = auth_tkt -# Not supported without an upgrade to "repoze.who.plugins.openid>=0.5.3" -#ax_optional = nickname=http://axschema.org/namePerson/friendly email=http://schema.openid.net/contact/email fullname=http://axschema.org/namePerson -#sreg_optional = nickname email fullname - [general] request_classifier = repoze.who.classifiers:default_request_classifier -# challenge_decider = repoze.who.classifiers:default_challenge_decider -challenge_decider = repoze.who.plugins.openid.classifiers:openid_challenge_decider +challenge_decider = repoze.who.classifiers:default_challenge_decider [identifiers] plugins = friendlyform;browser - openid auth_tkt [authenticators] plugins = auth_tkt - ckan.lib.authenticator:OpenIDAuthenticator ckan.lib.authenticator:UsernamePasswordAuthenticator [challengers] plugins = - openid friendlyform;browser # basicauth diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index 53d5cd8f6f2..4b3cbec1a62 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -347,11 +347,6 @@ def login(self, error=None): if 'error' in request.params: h.flash_error(request.params['error']) - if request.environ['SCRIPT_NAME'] and g.openid_enabled: - # #1662 restriction - log.warn('Cannot mount CKAN at a URL and login with OpenID.') - g.openid_enabled = False - if not c.user: came_from = request.params.get('came_from') if not came_from: @@ -383,9 +378,6 @@ def logged_in(self): return self.me() else: err = _('Login failed. Bad username or password.') - if g.openid_enabled: - err += _(' (Or if using OpenID, it hasn\'t been associated ' - 'with a user account.)') if h.asbool(config.get('ckan.legacy_templates', 'false')): h.flash_error(err) h.redirect_to(controller='user', diff --git a/ckan/lib/app_globals.py b/ckan/lib/app_globals.py index 38138e87815..098523fc5f6 100644 --- a/ckan/lib/app_globals.py +++ b/ckan/lib/app_globals.py @@ -55,7 +55,6 @@ 'ckan.plugins': {'type': 'split'}, # bool - 'openid_enabled': {'default': 'true', 'type' : 'bool'}, 'debug': {'default': 'false', 'type' : 'bool'}, 'ckan.debug_supress_header' : {'default': 'false', 'type' : 'bool'}, 'ckan.legacy_templates' : {'default': 'false', 'type' : 'bool'}, diff --git a/ckan/lib/authenticator.py b/ckan/lib/authenticator.py index cf6ed016694..fb6c66b019b 100644 --- a/ckan/lib/authenticator.py +++ b/ckan/lib/authenticator.py @@ -3,23 +3,10 @@ from zope.interface import implements from repoze.who.interfaces import IAuthenticator -from ckan.model import User, Session +from ckan.model import User log = logging.getLogger(__name__) -class OpenIDAuthenticator(object): - implements(IAuthenticator) - - def authenticate(self, environ, identity): - if 'repoze.who.plugins.openid.userid' in identity: - openid = identity['repoze.who.plugins.openid.userid'] - user = User.by_openid(openid) - if user is None or not user.is_active(): - return None - else: - return user.name - return None - class UsernamePasswordAuthenticator(object): implements(IAuthenticator) @@ -41,4 +28,3 @@ def authenticate(self, environ, identity): return user.name return None - diff --git a/ckan/lib/repoze_patch.py b/ckan/lib/repoze_patch.py deleted file mode 100644 index eb6b32ccc86..00000000000 --- a/ckan/lib/repoze_patch.py +++ /dev/null @@ -1,242 +0,0 @@ -from webob import Request, Response -from openid.consumer import consumer -from openid.extensions import sreg, ax - -# #1659 fix - logged_out_url prefixed with mount point -def get_full_path(path, environ): - if path.startswith('/'): - path = environ.get('SCRIPT_NAME', '') + path - return path - -def identify(self, environ): - """this method is called when a request is incoming. - - After the challenge has been called we might get here a response - from an openid provider. - - """ - - request = Request(environ) - logger = environ['repoze.who.logger'] - # #1659 fix - Use PATH_INFO rather than request.path as the former - # strips off the mount point. - path = environ['PATH_INFO'] - - # first test for logout as we then don't need the rest - if path == self.logout_handler_path: - res = Response() - # set forget headers - for a,v in self.forget(environ,{}): - res.headers.add(a,v) - res.status = 302 - url = self.logged_out_url + '?came_from=' + environ.get('came_from') - res.location = get_full_path(url, environ) - - environ['repoze.who.application'] = res - return {} - - identity = {} - - # first we check we are actually on the URL which is supposed to be the - # url to return to (login_handler_path in configuration) - # this URL is used for both: the answer for the login form and - # when the openid provider redirects the user back. - if path == self.login_handler_path: - - # in the case we are coming from the login form we should have - # an openid in here the user entered - open_id = request.params.get(self.openid_field, None) - if logger: - logger.debug('checking openid results for : %s ' %open_id) - - if open_id is not None: - open_id = open_id.strip() - - # we don't do anything with the openid we found ourselves but we put it in here - # to tell the challenge plugin to initiate the challenge - identity['repoze.whoplugins.openid.openid'] = open_id - environ['repoze.whoplugins.openid.openid'] = open_id - - # this part now is for the case when the openid provider redirects - # the user back. We should find some openid specific fields in the request. - mode=request.params.get("openid.mode", None) - if mode=="id_res": - oidconsumer = self.get_consumer(environ) - info = oidconsumer.complete(request.params, request.url) - if info.status == consumer.SUCCESS: - - fr = ax.FetchResponse.fromSuccessResponse(info) - if fr is not None: - items = chain(self.ax_require.items(), self.ax_optional.items()) - for key, value in items: - try: - identity['repoze.who.plugins.openid.' + key] = fr.get(value) - except KeyError: - pass - - fr = sreg.SRegResponse.fromSuccessResponse(info) - if fr is not None: - items = chain(self.sreg_require, self.sreg_optional) - for key in items: - try: - identity['repoze.who.plugins.openid.' + key] = fr.get(key) - except KeyError: - pass - - if logger: - logger.info('openid request successful for : %s ' %open_id) - - display_identifier = info.identity_url - - # remove this so that the challenger is not triggered again - del environ['repoze.whoplugins.openid.openid'] - - # store the id for the authenticator - identity['repoze.who.plugins.openid.userid'] = display_identifier - - # now redirect to came_from or the success page - self.redirect_to_logged_in(environ) - return identity - - # TODO: Do we have to check for more failures and such? - # - elif mode=="cancel": - # cancel is a negative assertion in the OpenID protocol, - # which means the user did not authorize correctly. - environ['repoze.whoplugins.openid.error'] = 'OpenID authentication failed.' - pass - return identity - -def redirect_to_logged_in(self, environ): - """redirect to came_from or standard page if login was successful""" - request = Request(environ) - came_from = request.params.get(self.came_from_field,'') - if came_from!='': - url = came_from - else: - url = get_full_path(self.logged_in_url, environ) - res = Response() - res.status = 302 - res.location = url - environ['repoze.who.application'] = res - -def challenge(self, environ, status, app_headers, forget_headers): - """the challenge method is called when the ``IChallengeDecider`` - in ``classifiers.py`` returns ``True``. This is the case for either a - ``401`` response from the client or if the key - ``repoze.whoplugins.openid.openidrepoze.whoplugins.openid.openid`` - is present in the WSGI environment. - The name of this key can be adjusted via the ``openid_field`` configuration - directive. - - The latter is the case when we are coming from the login page where the - user entered the openid to use. - - ``401`` can come back in any case and then we simply redirect to the login - form which is configured in the who configuration as ``login_form_url``. - - TODO: make the environment key to check also configurable in the challenge_decider. - - For the OpenID flow check `the OpenID library documentation - `_ - - """ - request = Request(environ) - logger = environ['repoze.who.logger'] - - # check for the field present, if not redirect to login_form - if not request.params.has_key(self.openid_field): - # redirect to login_form - res = Response() - res.status = 302 - res.location = get_full_path(self.login_form_url, environ)+"?%s=%s" %(self.came_from_field, request.url) - return res - - # now we have an openid from the user in the request - openid_url = request.params[self.openid_field] - if logger: - logger.debug('starting openid request for : %s ' %openid_url) - - try: - # we create a new Consumer and start the discovery process for the URL given - # in the library openid_request is called auth_req btw. - openid_request = self.get_consumer(environ).begin(openid_url) - - if len(self.ax_require.values()) or len(self.ax_optional.values()): - fetch_request = ax.FetchRequest() - for value in self.ax_require.values(): - fetch_request.add(ax.AttrInfo(value, required=True)) - for value in self.ax_optional.values(): - fetch_request.add(ax.AttrInfo(value, required=False)) - openid_request.addExtension(fetch_request) - - if len(self.sreg_require) or len(self.sreg_optional): - sreq = sreg.SRegRequest(required=self.sreg_require, optional=self.sreg_optional) - openid_request.addExtension(sreq) - - - except consumer.DiscoveryFailure, exc: - # eventually no openid server could be found - environ[self.error_field] = 'Error in discovery: %s' %exc[0] - if logger: - logger.info('Error in discovery: %s ' %exc[0]) - return self._redirect_to_loginform(environ) - except KeyError, exc: - # TODO: when does that happen, why does plone.openid use "pass" here? - environ[self.error_field] = 'Error in discovery: %s' %exc[0] - if logger: - logger.info('Error in discovery: %s ' %exc[0]) - return self._redirect_to_loginform(environ) - return None - - # not sure this can still happen but we are making sure. - # should actually been handled by the DiscoveryFailure exception above - if openid_request is None: - environ[self.error_field] = 'No OpenID services found for %s' %openid_url - if logger: - logger.info('No OpenID services found for: %s ' %openid_url) - return self._redirect_to_loginform(environ) - - # we have to tell the openid provider where to send the user after login - # so we need to compute this from our path and application URL - # we simply use the URL we are at right now (which is the form) - # this will be captured by the repoze.who identification plugin later on - # it will check if some valid openid response is coming back - # trust_root is the URL (realm) which will be presented to the user - # in the login process and should be your applications url - # TODO: make this configurable? - # return_to is the actual URL to be used for returning to this app - return_to = request.path_url # we return to this URL here - trust_root = request.application_url - if logger: - logger.debug('setting return_to URL to : %s ' %return_to) - - # TODO: usually you should check openid_request.shouldSendRedirect() - # but this might say you have to use a form redirect and I don't get why - # so we do the same as plone.openid and ignore it. - - # TODO: we might also want to give the application some way of adding - # extensions to this message. - redirect_url = openid_request.redirectURL(trust_root, return_to) - # # , immediate=False) - res = Response() - res.status = 302 - res.location = redirect_url - if logger: - logger.debug('redirecting to : %s ' %redirect_url) - - # now it's redirecting and might come back via the identify() method - # from the openid provider once the user logged in there. - return res - - -def _redirect_to_loginform(self, environ={}): - """redirect the user to the login form""" - res = Response() - res.status = 302 - q='' - ef = environ.get(self.error_field, None) - if ef is not None: - q='?%s=%s' %(self.error_field, ef) - res.location = get_full_path(self.login_form_url, environ)+q - return res diff --git a/ckan/tests/lib/test_authenticator.py b/ckan/tests/lib/test_authenticator.py index 8b2c6139f0f..4da951f63cd 100644 --- a/ckan/tests/lib/test_authenticator.py +++ b/ckan/tests/lib/test_authenticator.py @@ -68,58 +68,3 @@ def test_authenticate_fails_if_user_doesnt_exist(self): environ = {} identity = {'login': 'inexistent-user'} assert self.authenticate(environ, identity) is None - - -class TestOpenIDAuthenticator(object): - @classmethod - def setup_class(cls): - auth = authenticator.OpenIDAuthenticator() - cls.authenticate = auth.authenticate - - @classmethod - def teardown(cls): - ckan.model.repo.rebuild_db() - - def test_authenticate_succeeds_if_openid_is_correct(self): - environ = {} - openid = 'some-openid-key' - user = CreateTestData.create_user('a_user', **{'openid': openid}) - identity = {'login': user.name, - 'repoze.who.plugins.openid.userid': openid} - - username = self.authenticate(environ, identity) - assert username == user.name, username - - def test_authenticate_fails_if_openid_is_incorrect(self): - environ = {} - openid = 'wrong-openid-key' - user = CreateTestData.create_user('a_user') - identity = {'login': user.name, - 'repoze.who.plugins.openid.userid': openid} - - assert self.authenticate(environ, identity) is None - - def test_authenticate_fails_if_user_is_deleted(self): - environ = {} - openid = 'some-openid-key' - user = CreateTestData.create_user('a_user', **{'openid': openid}) - user.delete() - identity = {'login': user.name, - 'repoze.who.plugins.openid.userid': openid} - - assert self.authenticate(environ, identity) is None - - def test_authenticate_fails_if_user_is_pending(self): - environ = {} - openid = 'some-openid-key' - user = CreateTestData.create_user('a_user', **{'openid': openid}) - user.set_pending() - identity = {'login': user.name, - 'repoze.who.plugins.openid.userid': openid} - - assert self.authenticate(environ, identity) is None - - def test_authenticate_fails_if_user_have_no_openid(self): - environ = {} - identity = {} - assert self.authenticate(environ, identity) is None diff --git a/ckan/tests/test_coding_standards.py b/ckan/tests/test_coding_standards.py index 7c6aebbebd3..8126416ef66 100644 --- a/ckan/tests/test_coding_standards.py +++ b/ckan/tests/test_coding_standards.py @@ -408,7 +408,6 @@ class TestPep8(object): 'ckan/lib/activity_streams_session_extension.py', 'ckan/lib/alphabet_paginate.py', 'ckan/lib/app_globals.py', - 'ckan/lib/authenticator.py', 'ckan/lib/captcha.py', 'ckan/lib/cli.py', 'ckan/lib/create_test_data.py', @@ -435,7 +434,6 @@ class TestPep8(object): 'ckan/lib/package_saver.py', 'ckan/lib/plugins.py', 'ckan/lib/render.py', - 'ckan/lib/repoze_patch.py', 'ckan/lib/search/__init__.py', 'ckan/lib/search/index.py', 'ckan/lib/search/query.py', diff --git a/requirements.in b/requirements.in index 970cddb3b41..11547d7fe21 100644 --- a/requirements.in +++ b/requirements.in @@ -16,7 +16,6 @@ psycopg2==2.4.5 python-dateutil>=1.5.0,<2.0.0 pyutilib.component.core==4.5.3 repoze.who-friendlyform==1.0.8 -repoze.who.plugins.openid==0.5.3 repoze.who==2.0 requests==2.3.0 Routes==1.13 diff --git a/requirements.txt b/requirements.txt index b87a30ac995..da6217d1e43 100644 --- a/requirements.txt +++ b/requirements.txt @@ -33,7 +33,6 @@ pyutilib.component.core==4.5.3 repoze.lru==0.6 repoze.who==2.0 repoze.who-friendlyform==1.0.8 -repoze.who.plugins.openid==0.5.3 requests==2.3.0 simplejson==3.3.1 six==1.7.3 From e2a6089587f68b0a2e2f60c8d8c57c80c977511c Mon Sep 17 00:00:00 2001 From: Brook Elgie Date: Tue, 18 Nov 2014 16:44:51 +0000 Subject: [PATCH 2/7] [#1661] Remove unused test method --- ckan/tests/functional/test_user.py | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/ckan/tests/functional/test_user.py b/ckan/tests/functional/test_user.py index 53a3d9ae881..cd51bae8be7 100644 --- a/ckan/tests/functional/test_user.py +++ b/ckan/tests/functional/test_user.py @@ -127,26 +127,6 @@ def test_user_edit_not_logged_in(self): offset = url_for(controller='user', action='edit', id=username) res = self.app.get(offset, status=302) - def _login_openid(self, res): - # this requires a valid account on some openid provider - # (or for us to stub an open_id provider ...) - assert 'Please Sign In' in res - username = u'http://okfntest.myopenid.com' - fv = res.forms['user-login'] - fv['passurl'] = username - web.submit() - web.code(200) - assert 'You must sign in to authenticate to' in res - assert username in res - fv['password'] = u'okfntest' - res = fv.submit() - assert 'Please carefully verify whether you wish to trust' in res - fv = res.forms[0] - res = fv.submit('allow_once') - # at this point we should return - # but for some reason this does not work ... - return res - def test_perform_reset_user_password_link_key_incorrect(self): CreateTestData.create_user(name='jack', password='test1') # Make up a key - i.e. trying to hack this From 3db6fe82bef91035671ac9bdf1b3035819251877 Mon Sep 17 00:00:00 2001 From: Brook Elgie Date: Tue, 18 Nov 2014 16:45:35 +0000 Subject: [PATCH 3/7] [#1661] Remove python-openid requirement --- requirements.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index da6217d1e43..1e1efab85d9 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,7 +28,6 @@ passlib==1.6.2 pbr==0.8.2 psycopg2==2.4.5 python-dateutil==1.5 -python-openid==2.2.5 pyutilib.component.core==4.5.3 repoze.lru==0.6 repoze.who==2.0 From bead693b6b3d3e9b9d0580d4a9ed823d08040e98 Mon Sep 17 00:00:00 2001 From: Brook Elgie Date: Tue, 18 Nov 2014 16:51:06 +0000 Subject: [PATCH 4/7] [#1661] Add note to changelog --- CHANGELOG.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0f0ae3d53af..231867066e8 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -35,6 +35,8 @@ API changes and deprecations ``*``. To re-enable CORS, use the new ``ckan.cors`` settings detailed in the Config File Options documentation (:doc:`/maintaining/configuration`) +* OpenID support has been removed and is no longer supported. + Template changes ---------------- From b6dccb0dd879612fedf033f8907bf2dd3acec33c Mon Sep 17 00:00:00 2001 From: Brook Elgie Date: Tue, 18 Nov 2014 18:12:32 +0000 Subject: [PATCH 5/7] [#1661] Remove openid setting from test config --- test-core.ini | 2 -- 1 file changed, 2 deletions(-) diff --git a/test-core.ini b/test-core.ini index fedeb58f517..4a0aa72580f 100644 --- a/test-core.ini +++ b/test-core.ini @@ -90,8 +90,6 @@ ckan.datastore.enabled = 1 ckanext.stats.cache_enabled = 0 -openid_enabled = True - ckan.datasets_per_page = 20 ckan.activity_streams_email_notifications = True From eba7f2d77fc556302eb881cbd526ddc98ba85d77 Mon Sep 17 00:00:00 2001 From: Brook Elgie Date: Wed, 19 Nov 2014 11:11:52 +0000 Subject: [PATCH 6/7] [#1661] Amend comment referring to OpenID --- ckan/lib/base.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/ckan/lib/base.py b/ckan/lib/base.py index 9e0a800f1ec..0f704735ab9 100644 --- a/ckan/lib/base.py +++ b/ckan/lib/base.py @@ -298,11 +298,10 @@ def _identify_user_default(self): b) For API calls they may set a header with an API key. ''' - # environ['REMOTE_USER'] is set by repoze.who if it authenticates - # a user's cookie or OpenID. But repoze.who doesn't check the user - # (still) exists in our database - we need to do that here. (Another - # way would be with an userid_checker, but that would mean another db - # access. + # environ['REMOTE_USER'] is set by repoze.who if it authenticates a + # user's cookie. But repoze.who doesn't check the user (still) exists + # in our database - we need to do that here. (Another way would be + # with an userid_checker, but that would mean another db access. # See: http://docs.repoze.org/who/1.0/narr.html#module-repoze.who\ # .plugins.sql ) c.user = request.environ.get('REMOTE_USER', '') From 451bbd0577d0fefec08fc45a32e9780f1a8ddbbd Mon Sep 17 00:00:00 2001 From: Brook Elgie Date: Thu, 20 Nov 2014 17:19:21 +0000 Subject: [PATCH 7/7] [#1661] OpenID changelog wording --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e9aba810a5a..bea3c4d0bff 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -40,7 +40,7 @@ API changes and deprecations can be changed in the ``Repoze.who`` settings detailed in the Config File Options documentation (:ref:`who.httponly`). -* OpenID support has been removed and is no longer supported. +* The OpenID login option has been removed and is no longer supported. Template changes ----------------