From 885aff00ac6958a480441b4c53f6a26785c848ad Mon Sep 17 00:00:00 2001 From: Alvaro Lopez Garcia Date: Thu, 16 Jun 2016 11:40:29 +0200 Subject: [PATCH] oidc: deprecate grant_type argument The OpenID Connect grant type is something implicit to our plugins, so there is no need to pass it as a keyword argument, we can define this as a class argument. Moreover, it makes no sense to pass a grant_type that is different to the grant type that is being implemented. This change deprecates the keyword argument (showing a warning to the users if they're passing it) and checking if the grant_type is different to the one defined by the plugin itself. Change-Id: I4fffc2c651487d3240bbc8458ad728b06b0ffa64 --- keystoneauth1/exceptions/oidc.py | 6 ++- keystoneauth1/identity/v3/oidc.py | 44 ++++++++++++------- .../unit/identity/test_identity_v3_oidc.py | 32 ++++++++++++++ 3 files changed, 64 insertions(+), 18 deletions(-) diff --git a/keystoneauth1/exceptions/oidc.py b/keystoneauth1/exceptions/oidc.py index b7f282ae..28350176 100644 --- a/keystoneauth1/exceptions/oidc.py +++ b/keystoneauth1/exceptions/oidc.py @@ -16,7 +16,7 @@ from keystoneauth1.exceptions import auth_plugins __all__ = ( 'InvalidDiscoveryEndpoint', 'InvalidOidcDiscoveryDocument', 'OidcAccessTokenEndpointNotFound', 'OidcAuthorizationEndpointNotFound', - 'OidcPluginNotSupported', + 'OidcGrantTypeMissmatch', 'OidcPluginNotSupported', ) @@ -36,5 +36,9 @@ class OidcAuthorizationEndpointNotFound(auth_plugins.AuthPluginException): message = "OpenID Connect authorization endpoint not provided." +class OidcGrantTypeMissmatch(auth_plugins.AuthPluginException): + message = "Missmatch between OpenID Connect plugin and grant_type argument" + + class OidcPluginNotSupported(auth_plugins.AuthPluginException): message = "OpenID Connect grant type not supported by provider." diff --git a/keystoneauth1/identity/v3/oidc.py b/keystoneauth1/identity/v3/oidc.py index e58c4be7..dd8d0419 100644 --- a/keystoneauth1/identity/v3/oidc.py +++ b/keystoneauth1/identity/v3/oidc.py @@ -10,6 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. +import warnings + from positional import positional from keystoneauth1 import _utils as utils @@ -31,12 +33,16 @@ class _OidcBase(federation.FederationBaseAuth): ``http://openid.net/specs/openid-connect-core-1_0.html`` """ + grant_type = None + def __init__(self, auth_url, identity_provider, protocol, - client_id, client_secret, grant_type, + client_id, client_secret, access_token_type, scope="openid profile", access_token_endpoint=None, - discovery_endpoint=None, **kwargs): + discovery_endpoint=None, + grant_type=None, + **kwargs): """The OpenID Connect plugin expects the following. :param auth_url: URL of the Identity Service @@ -55,14 +61,6 @@ class _OidcBase(federation.FederationBaseAuth): :param client_secret: OAuth 2.0 Client Secret :type client_secret: string - :param grant_type: OpenID Connect grant type, it represents the flow - that is used to talk to the OP. Valid values are: - "authorization_code", "refresh_token", or - "password". If a discovery document is being used, - this class will check if the provided value is - supported by the provider. - :type grant_type: string - :param access_token_type: OAuth 2.0 Authorization Server Introspection token type, it is used to decide which type of token will be used when processing token @@ -101,7 +99,18 @@ class _OidcBase(federation.FederationBaseAuth): self.access_token_type = access_token_type self.scope = scope - self.grant_type = grant_type + if grant_type is not None: + if grant_type != self.grant_type: + raise exceptions.OidcGrantTypeMissmatch() + warnings.warn("Passing grant_type as an argument has been " + "deprecated as it is now defined in the plugin " + "itself. You should stop passing this argument " + "to the plugin, as it will be ignored, since you " + "cannot pass a free text string as a grant_type. " + "This argument will be dropped from the plugin in " + "July 2017 or with the next major release of " + "keystoneauth (3.0.0)", + DeprecationWarning) def _get_discovery_document(self, session): """Get the contents of the OpenID Connect Discovery Document. @@ -231,13 +240,16 @@ class _OidcBase(federation.FederationBaseAuth): class OidcPassword(_OidcBase): """Implementation for OpenID Connect Resource Owner Password Credential.""" + grant_type = "password" + @positional(4) def __init__(self, auth_url, identity_provider, protocol, client_id, client_secret, access_token_endpoint=None, discovery_endpoint=None, - grant_type='password', access_token_type='access_token', - username=None, password=None, **kwargs): + access_token_type='access_token', + username=None, password=None, + **kwargs): """The OpenID Password plugin expects the following. :param username: Username used to authenticate @@ -254,7 +266,6 @@ class OidcPassword(_OidcBase): client_secret=client_secret, access_token_endpoint=access_token_endpoint, discovery_endpoint=discovery_endpoint, - grant_type=grant_type, access_token_type=access_token_type, **kwargs) self.username = username @@ -297,12 +308,13 @@ class OidcPassword(_OidcBase): class OidcAuthorizationCode(_OidcBase): """Implementation for OpenID Connect Authorization Code.""" + grant_type = 'authorization_code' + @positional(4) def __init__(self, auth_url, identity_provider, protocol, client_id, client_secret, access_token_endpoint=None, discovery_endpoint=None, - grant_type='authorization_code', access_token_type='access_token', redirect_uri=None, code=None, **kwargs): """The OpenID Authorization Code plugin expects the following. @@ -322,7 +334,6 @@ class OidcAuthorizationCode(_OidcBase): client_secret=client_secret, access_token_endpoint=access_token_endpoint, discovery_endpoint=discovery_endpoint, - grant_type=grant_type, access_token_type=access_token_type, **kwargs) self.redirect_uri = redirect_uri @@ -390,7 +401,6 @@ class OidcAccessToken(_OidcBase): client_id=None, client_secret=None, access_token_endpoint=None, - grant_type=None, access_token_type=None, **kwargs) self.access_token = access_token diff --git a/keystoneauth1/tests/unit/identity/test_identity_v3_oidc.py b/keystoneauth1/tests/unit/identity/test_identity_v3_oidc.py index 3afaa094..7684995a 100644 --- a/keystoneauth1/tests/unit/identity/test_identity_v3_oidc.py +++ b/keystoneauth1/tests/unit/identity/test_identity_v3_oidc.py @@ -11,6 +11,7 @@ # under the License. import uuid +import warnings from six.moves import urllib @@ -48,6 +49,33 @@ class BaseOIDCTests(object): self.DISCOVERY_URL = ('https://localhost:8020/oidc/.well-known/' 'openid-configuration') + self.GRANT_TYPE = None + + def test_grant_type_and_plugin_missmatch(self): + self.assertRaises( + exceptions.OidcGrantTypeMissmatch, + self.plugin.__class__, + self.AUTH_URL, + self.IDENTITY_PROVIDER, + self.PROTOCOL, + client_id=self.CLIENT_ID, + client_secret=self.CLIENT_SECRET, + grant_type=uuid.uuid4().hex + ) + + def test_can_pass_grant_type_but_warning_is_issued(self): + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + self.plugin.__class__( + self.AUTH_URL, + self.IDENTITY_PROVIDER, + self.PROTOCOL, + client_id=self.CLIENT_ID, + client_secret=self.CLIENT_SECRET, + grant_type=self.GRANT_TYPE) + assert len(w) == 1 + assert issubclass(w[-1].category, DeprecationWarning) + assert "grant_type" in str(w[-1].message) def test_discovery_not_found(self): self.requests_mock.get("http://not.found", @@ -156,6 +184,8 @@ class OIDCPasswordTests(BaseOIDCTests, utils.TestCase): def setUp(self): super(OIDCPasswordTests, self).setUp() + self.GRANT_TYPE = 'password' + self.plugin = oidc.OidcPassword( self.AUTH_URL, self.IDENTITY_PROVIDER, @@ -228,6 +258,8 @@ class OIDCAuthorizationGrantTests(BaseOIDCTests, utils.TestCase): def setUp(self): super(OIDCAuthorizationGrantTests, self).setUp() + self.GRANT_TYPE = 'authorization_code' + self.plugin = oidc.OidcAuthorizationCode( self.AUTH_URL, self.IDENTITY_PROVIDER,