Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes to make the library work correctly, some fixes to the "simple" example and some internal documentation. #803

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
5 changes: 5 additions & 0 deletions oidc_example/simple_op/src/provider/authn/user_pass.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ def __call__(self, *args, **kwargs):
state=json.dumps(kwargs),
**self.kwargs))

"""
Checks if the user can be authenticated with the password provided

:param kwargs:
"""
def verify(self, *args, **kwargs):
username = kwargs["username"]
if username in self.user_db and self.user_db[username] == kwargs[
Expand Down
28 changes: 24 additions & 4 deletions oidc_example/simple_op/src/provider/server/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from functools import partial
from functools import wraps
from urllib import parse as urlparse

import cherrypy
import yaml
from cherrypy import wsgiserver
Expand Down Expand Up @@ -70,13 +69,16 @@ def wrapper(environ, start_response):

url = "{base_url}?{query_string}".format(
base_url="/authorization",
query_string=kwargs["state"]["query"])
query_string=urlparse.urlparse(environ.get("HTTP_REFERER")).query)
response = SeeOther(url, headers=[(set_cookie, cookie_value)])
return response(environ, start_response)

else: # Unsuccessful authentication
url = "{base_url}?{query_string}".format(
base_url="/authorization",
query_string=kwargs["state"]["query"])
query_string=urlparse.urlparse(environ.get("HTTP_REFERER")).query)
response = SeeOther(url, headers=[(set_cookie, cookie_value)])
return response(environ, start_response)
response = SeeOther(url)
return response(environ, start_response)

Expand All @@ -93,7 +95,11 @@ def pyoidcMiddleware(func):
def wrapper(environ, start_response):
data = get_or_post(environ)
cookies = environ.get("HTTP_COOKIE", "")
resp = func(request=data, cookie=cookies)
if(environ.get("REQUEST_URI", "") == "/token"):
# This is correct at least for our case which is the authorization code flow.
resp = func(request=data, cookie=cookies, authn=environ.get("HTTP_AUTHORIZATION", ""))
else:
resp = func(request=data, cookie=cookies)
return resp(environ, start_response)

return wrapper
Expand All @@ -107,14 +113,24 @@ def resp2flask(resp):
return resp.message, resp.status, resp.headers


"""
Creates an AuthnBroker with the authentication methods set in settings.yaml.

:returns an AuthnBroker and the routing.
"""
def setup_authentication_methods(authn_config, template_env):
"""Add all authentication methods specified in the configuration."""
routing = {}
ac = AuthnBroker()
for authn_method in authn_config:

# Create instance of the appropiate auth class.
cls = make_cls_from_name(authn_method["class"])
instance = cls(template_env=template_env, **authn_method["kwargs"])

# Adds the auth method name to the broker.
ac.add(authn_method["acr"], instance)

routing[instance.url_endpoint] = VerifierMiddleware(instance)

return ac, routing
Expand All @@ -123,6 +139,8 @@ def setup_authentication_methods(authn_config, template_env):
def setup_endpoints(provider):
"""Setup the OpenID Connect Provider endpoints."""
app_routing = {}

# Each provider.***** is a pointer to a function
endpoints = [
AuthorizationEndpoint(
pyoidcMiddleware(provider.authorization_endpoint)),
Expand Down Expand Up @@ -204,6 +222,8 @@ def main():
client_db = {}
session_db = create_session_db(issuer,
secret=rndstr(32), password=rndstr(32))

# verify_client is a pointer to that function which is used in the token endpoint.
provider = Provider(issuer, session_db, client_db, authn_broker,
userinfo, AuthzHandling(), verify_client, None)
provider.baseurl = issuer
Expand Down
2 changes: 2 additions & 0 deletions oidc_example/simple_rp/src/rp.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ def authenticate(self, uid):
]
_, keyjar, _ = build_keyjar(keys)
cherrypy.session["client"] = Client(verify_ssl=self.verify_ssl, keyjar=keyjar)
cherrypy.session["client"].token_endpoint = "https://localhost/token"
cherrypy.session["client"].userinfo_endpoint = "https://localhost/userinfo"

# webfinger+registration
self.rp.register_with_dynamic_provider(cherrypy.session, uid)
Expand Down
2 changes: 2 additions & 0 deletions src/oic/oauth2/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,8 @@ def construct_AuthorizationRequest(
else:
request_args = {}

request_args.update(kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the content should be in the request_args, the kwargs can potentially contain bad arguments.

Choose a reason for hiding this comment

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

My problem is that if I don't add the request args manually from kwargs these arguments like client_id, redirect_uri passed to the oic.oic. construct_AuthenticationRequest method are never added to the authentication request. So, if I'm not doing anything else wrong, there is a problem when picking the args passed to this method because if I run my app with the authenticationRequest method detailed below without my changes the redirect_uri arg in the authentication request made to the OP is equal to "h". When I use the python debug console to print session["client"].registration_response["redirect_uris"][0] its value is 'https://localhost:8080/authenticateDigitelTS' and therefore it's correct.

def authenticationRequest(self, session):
        """
        Sends a request to the OP to authenticate the user that made this request.

        :param session: a CherryPy Session with the session of the user that made the request.

        :return a RegistrationResponse instance with the response.
        """

        session["state"] = rndstr()
        session["nonce"] = rndstr()
        args = {
            "client_id": session["client"].client_id,
            "response_type": session["client"].response_type,
            "scope": session["client"].scope,
            "nonce": session["nonce"],
            "redirect_uri": session["client"].registration_response["redirect_uris"][0],
            "state": session["state"]
        }

        auth_request = session["client"].construct_AuthorizationRequest(**args)
        # Send the request to the authorization endpoint.
        loginPseudoURL = auth_request.request(session["client"].authorization_endpoint)

        return loginPseudoURL

I don't know if the problem is because of these arguments never being added to request_args in oic.oauth2.construct_AuthorizationRequest or there is an error in any other place because I don't know all the implementation details of the library that well. Maybe you can point me in the right direction. Thanks for your help, have a good day.

PD: I will check google documentation conventions for future PRs


if "client_id" not in request_args:
request_args["client_id"] = self.client_id
elif not request_args["client_id"]:
Expand Down
2 changes: 1 addition & 1 deletion src/oic/oic/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ def verify_id_token(instance, check_hash=False, **kwargs):

if "keyjar" in kwargs:
try:
if _body["iss"] not in kwargs["keyjar"]:
if _body["iss"] not in kwargs["iss"]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is checking if we have the needed key to make the signature and/or encryption.

raise ValueError("Unknown issuer")
except KeyError:
raise MissingRequiredAttribute("iss")
Expand Down
3 changes: 3 additions & 0 deletions src/oic/oic/provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,9 @@ def __init__(
logout_path="",
settings: PyoidcSettings = None,
):
"""
:param name the iss, which is the URI of the OP.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather use the google style conventions.

Choose a reason for hiding this comment

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

I will take a look at it, thanks for your advice.

"""
self.settings = settings or OicProviderSettings()
if verify_ssl is not None:
warnings.warn(
Expand Down