From 2208db5630408d1c515732309fc5f260a72780e1 Mon Sep 17 00:00:00 2001 From: Ross Patterson Date: Mon, 3 Jan 2022 16:26:35 -0800 Subject: [PATCH] fix(auth): Missing JWT plugin activation upgrade Because token generation has been moved into `updateCredentials(...)` [we need an upgrade step](https://github.com/plone/plone.restapi/pull/1303#issuecomment-1002756856) that enables the JWT token plugin for that PAS plugin interface on existing installations in order for authentication to work as before. Also fixes existing plugins outside of a Plone portal that have been configured to use the keyring. I tested this locally by: 1. erasing my local data (ZODB) 2. checking out `master` in the `plone/volto` repo 3. running buildout, including `plonesite` in the API to re-create the portal 4. adding a test user in the Plone portal through the Volto UI 5. add `mr.developer` sources and checkouts in the API buildout 6. disable `plonesite` in the API buildout 7. run buildout to update the code to the PR branches 8. test all the upgrade error conditions around login logout 9. run the `v0006 -> v0007` upgrade step for `plone.restapi:default` 10. confirm all the upgrade error conditions around login logout have been resolved Not that this doesn't address the issue of [existing Zope root `/acl_users/` cookie login set up](https://github.com/plone/plone.restapi/pull/1304#issuecomment-1002759581). --- src/plone/restapi/pas/__init__.py | 27 +++++++++++ src/plone/restapi/pas/plugin.py | 33 +++++++++++--- .../restapi/profiles/default/metadata.xml | 2 +- src/plone/restapi/services/auth/login.py | 22 ++++++++- src/plone/restapi/setuphandlers.py | 23 +--------- src/plone/restapi/tests/test_addons.py | 9 ++-- src/plone/restapi/tests/test_auth.py | 3 +- src/plone/restapi/upgrades/configure.zcml | 11 +++++ src/plone/restapi/upgrades/to0007.py | 45 +++++++++++++++++++ 9 files changed, 143 insertions(+), 32 deletions(-) create mode 100644 src/plone/restapi/upgrades/to0007.py diff --git a/src/plone/restapi/pas/__init__.py b/src/plone/restapi/pas/__init__.py index 6fd571e370..4fc09f6934 100644 --- a/src/plone/restapi/pas/__init__.py +++ b/src/plone/restapi/pas/__init__.py @@ -3,7 +3,12 @@ """ from App import Management +from Products.CMFCore.utils import getToolByName +from Products.CMFPlone import interfaces as plone_ifaces from Products import PluggableAuthService # noqa, Ensure PAS patch in place +from Products.PluggableAuthService.interfaces import authservice as authservice_ifaces + +import Acquisition _orig_manage_zmi_logout = Management.Navigation.manage_zmi_logout @@ -20,3 +25,25 @@ def manage_zmi_logout(self, REQUEST, RESPONSE): # Undo the HTTP `Authorization: Basic ...` assumptions del RESPONSE.headers["WWW-Authenticate"] RESPONSE.setStatus(200) + + +def iter_ancestor_pas(context): + """ + Walk up the ZODB OFS returning Pluggableauthservice `./acl_users/` for each level. + """ + uf_parent = Acquisition.aq_inner(context) + while True: + is_plone_site = plone_ifaces.IPloneSiteRoot.providedBy(uf_parent) + uf = getToolByName(uf_parent, "acl_users", default=None) + + # Skip ancestor contexts to which we don't/can't apply + if uf is None or not authservice_ifaces.IPluggableAuthService.providedBy(uf): + uf_parent = Acquisition.aq_parent(uf_parent) + continue + + yield uf, is_plone_site + + # Go up one more level + if uf_parent is uf_parent.getPhysicalRoot(): + break + uf_parent = Acquisition.aq_parent(uf_parent) diff --git a/src/plone/restapi/pas/plugin.py b/src/plone/restapi/pas/plugin.py index a60f672122..1b77456f99 100644 --- a/src/plone/restapi/pas/plugin.py +++ b/src/plone/restapi/pas/plugin.py @@ -17,12 +17,15 @@ from Products.PluggableAuthService.interfaces.plugins import ICredentialsUpdatePlugin from Products.PluggableAuthService.interfaces.plugins import ICredentialsResetPlugin from Products.PluggableAuthService.plugins.BasePlugin import BasePlugin -from zope.component import getUtility +from zope import component from zope.interface import implementer import jwt +import logging import time +logger = logging.getLogger(__name__) + manage_addJWTAuthenticationPlugin = PageTemplateFile( "add_plugin", globals(), __name__="manage_addJWTAuthenticationPlugin" @@ -209,9 +212,29 @@ def manage_updateConfig(self, REQUEST): % (self.absolute_url(), "Configuration+updated.") ) - def _decode_token(self, token, verify=True): + def _use_keyring(self): + """ + Determine whether to use the keyring for this plugin, logging error conditions. + """ + use_keyring = False + keyring_manager = None if self.use_keyring: - manager = getUtility(IKeyManager) + keyring_manager = component.queryUtility(IKeyManager) + if keyring_manager is None: + logger.error( + "Plugin configured to use keyring but no utility registered: %s", + "/".join(self.getPhysicalPath()), + ) + else: + use_keyring = True + return use_keyring, keyring_manager + + def _decode_token(self, token, verify=True): + """ + Decode the given JWT token using the keyring if configured to do so. + """ + use_keyring, manager = self._use_keyring() + if use_keyring: for secret in manager["_system"]: if secret is None: continue @@ -230,8 +253,8 @@ def _jwt_decode(self, token, secret, verify=True): pass def _signing_secret(self): - if self.use_keyring: - manager = getUtility(IKeyManager) + use_keyring, manager = self._use_keyring() + if use_keyring: return manager.secret() + self._path() if not self._secret: self._secret = GenerateSecret() diff --git a/src/plone/restapi/profiles/default/metadata.xml b/src/plone/restapi/profiles/default/metadata.xml index 81970e34ed..1e4872e498 100644 --- a/src/plone/restapi/profiles/default/metadata.xml +++ b/src/plone/restapi/profiles/default/metadata.xml @@ -1,4 +1,4 @@ - 0006 + 0007 diff --git a/src/plone/restapi/services/auth/login.py b/src/plone/restapi/services/auth/login.py index 21fa9fa6b5..e9014b8b3d 100644 --- a/src/plone/restapi/services/auth/login.py +++ b/src/plone/restapi/services/auth/login.py @@ -86,7 +86,27 @@ def reply(self): ) login_view._post_login() - return {"token": self.request[plugin.cookie_name]} + response = {} + if plugin.cookie_name in self.request: + response["token"] = self.request[plugin.cookie_name] + else: + self.request.response.setStatus(501) + message = ( + "JWT authentication token not created, plugin probably not activated " + "for `ICredentialsUpdatePlugin`" + ) + logger.error( + "%s: %s", + message, + "/".join(plugin.getPhysicalPath()), + ) + return dict( + error=dict( + type="Login failed", + message=message, + ) + ) + return response def _find_userfolder(self, userid): """Try to find a user folder that contains a user with the given diff --git a/src/plone/restapi/setuphandlers.py b/src/plone/restapi/setuphandlers.py index 7077c52149..0872e179b6 100644 --- a/src/plone/restapi/setuphandlers.py +++ b/src/plone/restapi/setuphandlers.py @@ -1,13 +1,7 @@ -from Acquisition import aq_inner -from Acquisition import aq_parent +from plone.restapi import pas from plone.restapi.pas.plugin import JWTAuthenticationPlugin -from Products.CMFCore.utils import getToolByName -from Products.CMFPlone import interfaces as plone_ifaces from Products.CMFPlone.interfaces import INonInstallable from Products.PluggableAuthService.interfaces import plugins as plugins_ifaces -from Products.PluggableAuthService.interfaces.authservice import ( - IPluggableAuthService, -) # noqa: E501 from Products.PluggableAuthService.plugins import CookieAuthHelper from Products.PluggableAuthService.plugins import HTTPBasicAuthHelper from zope.component.hooks import getSite @@ -40,15 +34,7 @@ def install_pas_plugin(context): Usually this means it is installed into Plone and into the Zope root. """ - uf_parent = aq_inner(context) - while True: - is_plone_site = plone_ifaces.IPloneSiteRoot.providedBy(uf_parent) - uf = getToolByName(uf_parent, "acl_users", default=None) - - # Skip ancestor contexts to which we don't/can't apply - if uf is None or not IPluggableAuthService.providedBy(uf): - uf_parent = aq_parent(uf_parent) - continue + for uf, is_plone_site in pas.iter_ancestor_pas(context): # Add the API token plugin if not already installed at this level if "jwt_auth" not in uf: @@ -111,11 +97,6 @@ def install_pas_plugin(context): basic_plugin.id, ) - # Go up one more level - if uf_parent is uf_parent.getPhysicalRoot(): - break - uf_parent = aq_parent(uf_parent) - def post_install_default(context): """Post install of default profile""" diff --git a/src/plone/restapi/tests/test_addons.py b/src/plone/restapi/tests/test_addons.py index 9fd449e7ee..36b5e40f40 100644 --- a/src/plone/restapi/tests/test_addons.py +++ b/src/plone/restapi/tests/test_addons.py @@ -116,12 +116,15 @@ def _get_upgrade_info(self): # Set need upgrade state self.ps.setLastVersionForProfile("plone.restapi:default", "0002") transaction.commit() + # FIXME: At least the `newVersion` should be extracted from + # `./profiles/default/metadata.xml` so that this test isn't constantly changing + # for unrelated code changes. self.assertEqual( { "available": True, "hasProfile": True, "installedVersion": "0002", - "newVersion": "0006", + "newVersion": "0007", "required": True, }, _get_upgrade_info(self), @@ -135,8 +138,8 @@ def _get_upgrade_info(self): { "available": False, "hasProfile": True, - "installedVersion": "0006", - "newVersion": "0006", + "installedVersion": "0007", + "newVersion": "0007", "required": False, }, _get_upgrade_info(self), diff --git a/src/plone/restapi/tests/test_auth.py b/src/plone/restapi/tests/test_auth.py index e619e0ab75..2db395dfca 100644 --- a/src/plone/restapi/tests/test_auth.py +++ b/src/plone/restapi/tests/test_auth.py @@ -86,7 +86,8 @@ def test_login_with_zope_user_fails_without_pas_plugin(self): res = service.reply() self.assertIn("error", res) self.assertEqual( - "JWT authentication plugin not installed.", res["error"]["message"] + "jwt authentication plugin not installed", + res["error"]["message"].lower(), ) self.assertNotIn("token", res) diff --git a/src/plone/restapi/upgrades/configure.zcml b/src/plone/restapi/upgrades/configure.zcml index 66bb67b0d7..88b9b86425 100644 --- a/src/plone/restapi/upgrades/configure.zcml +++ b/src/plone/restapi/upgrades/configure.zcml @@ -69,4 +69,15 @@ handler="plone.restapi.upgrades.to0006.rename_iface_to_name_in_blocks_behavior" /> + + diff --git a/src/plone/restapi/upgrades/to0007.py b/src/plone/restapi/upgrades/to0007.py new file mode 100644 index 0000000000..402b2faad6 --- /dev/null +++ b/src/plone/restapi/upgrades/to0007.py @@ -0,0 +1,45 @@ +""" +GenericSetup profile upgrades from version 0006 to 0007. +""" + +from plone.restapi import pas +from plone.restapi.pas import plugin +from Products.CMFCore.utils import getToolByName +from Products.PluggableAuthService.interfaces import plugins as plugins_ifaces + +import logging + +logger = logging.getLogger(__name__) + + +def enable_new_pas_plugin_interfaces(context): + """ + Enable new PAS plugin interfaces. + + After correcting/completing the PAS plugin interfaces, those interfaces need to be + enabled for existing functionality to continue working. + """ + portal = getToolByName(context, "portal_url").getPortalObject() + for uf, is_plone_site in pas.iter_ancestor_pas(portal): + for jwt_plugin in uf.objectValues(plugin.JWTAuthenticationPlugin.meta_type): + if not is_plone_site and jwt_plugin.use_keyring: + logger.info( + "Disabling keyring for plugin outside of Plone: %s", + "/".join(jwt_plugin.getPhysicalPath()) + ) + jwt_plugin.use_keyring = False + for new_iface in ( + plugins_ifaces.ICredentialsUpdatePlugin, + plugins_ifaces.ICredentialsResetPlugin, + ): + active_plugin_ids = [ + active_plugin_id for active_plugin_id, _ in + uf.plugins.listPlugins(new_iface) + ] + if jwt_plugin.id not in active_plugin_ids: + logger.info( + "Activating PAS interface %s: %s", + new_iface.__name__, + "/".join(jwt_plugin.getPhysicalPath()) + ) + uf.plugins.activatePlugin(new_iface, jwt_plugin.id)