Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 25 additions & 18 deletions src/oic/oic/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,7 @@ def do_end_session_request(self, request=EndSessionRequest, scope="",
def user_info_request(self, method="GET", state="", scope="", **kwargs):
uir = UserInfoRequest()
logger.debug("[user_info_request]: kwargs:%s" % (sanitize(kwargs),))
token = None
if "token" in kwargs:
if kwargs["token"]:
uir["access_token"] = kwargs["token"]
Expand All @@ -773,12 +774,11 @@ def user_info_request(self, method="GET", state="", scope="", **kwargs):
kwargs["behavior"] = "use_authorization_header"
else:
# What to do ? Need a callback
token = None
pass
elif "access_token" in kwargs and kwargs["access_token"]:
uir["access_token"] = kwargs["access_token"]
del kwargs["access_token"]
token = None
else:
elif state:
token = self.grant[state].get_token(scope)

if token.is_valid():
Expand Down Expand Up @@ -808,17 +808,19 @@ def user_info_request(self, method="GET", state="", scope="", **kwargs):
if "behavior" in kwargs:
_behav = kwargs["behavior"]
_token = uir["access_token"]
_ttype = ''
try:
_ttype = kwargs["token_type"]
except KeyError:
try:
_ttype = token.token_type
except AttributeError:
raise MissingParameter("Unspecified token type")
if token:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same goes here: if token is not None:

try:
_ttype = token.token_type
except AttributeError:
raise MissingParameter("Unspecified token type")

if 'as_query_parameter' == _behav:
method = 'GET'
else:
elif token:
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Comparison to None should be explicit.
  • In tests, this condition is always true, so a test with token = None should be added.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The comparison is still not explicit. It believe that it should be elif token is not None: as token is previously assigned None.

Copy link
Contributor Author

@rohe rohe Sep 10, 2017

Choose a reason for hiding this comment

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

I don't think that is good enough. token='' should be handled the same way as token=None
hence the use of elif token which implicitly include token is not ''.
And I'd rather not use elif token is not None and token is not ''

Copy link
Collaborator

Choose a reason for hiding this comment

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

And is there any case where token would be an empty string?
If yes, then OK, leave the code as is.

# use_authorization_header, token_in_message_body
if "use_authorization_header" in _behav:
token_header = "{type} {token}".format(
Expand Down Expand Up @@ -899,12 +901,13 @@ def do_user_info_request(self, method="POST", state="", scope="openid",
if 'error' in res: # Error response
res = UserInfoErrorResponse(**res.to_dict())

# Verify userinfo sub claim against what's returned in the ID Token
idt = self.grant[state].get_id_token()
if idt:
if idt['sub'] != res['sub']:
raise SubMismatch(
'Sub identifier not the same in userinfo and Id Token')
if state:
# Verify userinfo sub claim against what's returned in the ID Token
idt = self.grant[state].get_id_token()
if idt:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to never be triggered in tests.

if idt['sub'] != res['sub']:
raise SubMismatch(
'Sub identifier not the same in userinfo and Id Token')

self.store_response(res, _txt)

Expand Down Expand Up @@ -1064,12 +1067,16 @@ def fetch_distributed_claims(self, userinfo, callback=None):
if "endpoint" in spec:
if "access_token" in spec:
_uinfo = self.do_user_info_request(
token=spec["access_token"],
method='GET', token=spec["access_token"],
userinfo_endpoint=spec["endpoint"])
else:
_uinfo = self.do_user_info_request(
token=callback(csrc),
userinfo_endpoint=spec["endpoint"])
if callback:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part is not covered by tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the recent changes meet your requirements ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for not responding earlier. I am having a busy week at work. I will have a look today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here again, comparison to None and test without callback is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK now ? Need to get this out the door soon.

_uinfo = self.do_user_info_request(
method='GET', token=callback(spec['endpoint']),
userinfo_endpoint=spec["endpoint"])
else:
_uinfo = self.do_user_info_request(
method='GET', userinfo_endpoint=spec["endpoint"])

claims = [value for value, src in
userinfo["_claim_names"].items() if src == csrc]
Expand Down
100 changes: 98 additions & 2 deletions tests/test_oic.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from jwkest.jws import alg2keytype
from jwkest.jws import left_hash
from jwkest.jwt import JWT
from requests import Response

from oic.oauth2.exception import OtherError
from oic.oic import DEF_SIGN_ALG
Expand Down Expand Up @@ -756,7 +757,8 @@ def test_request_1():
'.eyJzdGF0ZSI6ImZvb2JhciIsImlzcyI6Inp2bWk4UGdJbURiOSIsImF1ZCI6I' \
'mh0dHBzOi8vcnAuY2VydGlmaWNhdGlvbi5vcGVuaWQubmV0OjgwODAvbm9kZS1' \
'vcGVuaWQtY2xpZW50L3JwLXJlcXVlc3RfdXJpLXVuc2lnbmVkIiwiY2xpZW50X' \
'2lkIjoienZtaThQZ0ltRGI5In0.&client_id=zvmi8PgImDb9&scope=openid&response_type=code'
'2lkIjoienZtaThQZ0ltRGI5In0.&client_id=zvmi8PgImDb9&scope=openid' \
'&response_type=code'

req = srv.parse_authorization_request(query=areq)

Expand All @@ -767,7 +769,8 @@ def test_request_duplicate_state():
srv = Server()
srv.keyjar = KEYJ

areq = 'redirect_uri=https%3A%2F%2Fnode-openid-client.dev%2Fcb&state=barf&request' \
areq = 'redirect_uri=https%3A%2F%2Fnode-openid-client.dev%2Fcb&state=barf' \
'&request' \
'=eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0' \
'.eyJzdGF0ZSI6ImZvb2JhciIsImlzcyI6Inp2bWk4UGdJbURiOSIsImF1ZCI6Imh0dHBzOi8v' \
'cnAuY2VydGlmaWNhdGlvbi5vcGVuaWQubmV0OjgwODAvbm9kZS1vcGVuaWQtY2xpZW50L3JwL' \
Expand All @@ -778,3 +781,96 @@ def test_request_duplicate_state():

assert req['state'] == 'foobar'
assert req['redirect_uri'] == 'https://node-openid-client.dev/cb'


def test_do_userinfo_request_no_state_or_token():
""" Mirrors the first lines in do_userinfo_request"""
client = Client(CLIENT_ID, client_authn_method=CLIENT_AUTHN_METHOD)

method = "GET"
state = ""
scope = "openid"
request = "openid"
kwargs = {"request": request,
"userinfo_endpoint": 'http://example.com/userinfo'}

path, body, method, h_args = client.user_info_request(method, state,
scope, **kwargs)

assert path == 'http://example.com/userinfo'
assert h_args == {}
assert body is None
assert method == 'GET'


def test_do_userinfo_request_token_no_state():
""" Mirrors the first lines in do_userinfo_request"""
client = Client(CLIENT_ID, client_authn_method=CLIENT_AUTHN_METHOD)

method = "GET"
state = ""
scope = "openid"
request = "openid"
kwargs = {"request": request,
"userinfo_endpoint": 'http://example.com/userinfo',
"token": "abcdefgh"}

path, body, method, h_args = client.user_info_request(method, state,
scope, **kwargs)

assert path == 'http://example.com/userinfo'
assert h_args == {'headers': {'Authorization': 'Bearer abcdefgh'}}
assert method == 'GET'
assert body is None


def test_do_userinfo_request_explicit_token_none():
""" Mirrors the first lines in do_userinfo_request"""
client = Client(CLIENT_ID, client_authn_method=CLIENT_AUTHN_METHOD)

method = "GET"
state = ""
scope = "openid"
request = "openid"
kwargs = {"request": request,
"userinfo_endpoint": 'http://example.com/userinfo',
"token": None}

path, body, method, h_args = client.user_info_request(method, state,
scope, **kwargs)

assert path == 'http://example.com/userinfo'
assert h_args == {}
assert method == 'GET'
assert body is None


def token_callback(endp):
return 'abcdef'


def fake_request(*args, **kwargs):
r = Response()
r.status_code = 200
r._content = b'{"shoe_size": 12}'
r.headers = {'content-type': 'application/json'}
return r


def test_fetch_distributed_claims_with_callback():
""" Mirrors the first lines in do_userinfo_request"""
client = Client(CLIENT_ID, client_authn_method=CLIENT_AUTHN_METHOD)

client.http_request = fake_request
userinfo = {
'sub': 'foobar',
'_claim_names': {'shoe_size': 'src1'},
'_claim_sources': {
"src1": {
"endpoint": "https://bank.example.com/claim_source"}}
}

_ui = client.fetch_distributed_claims(userinfo, token_callback)

assert _ui['shoe_size'] == 12
assert _ui['sub'] == 'foobar'