Empty lists not indexable

[andrewkrug]

The following line https://github.com/OpenIDC/pyoidc/blob/master/src/oic/oauth2/message.py#L322 will raise an indexing exception if a userinfo endpoint returns an attribute who's key, value is an empty list.

Example:

nicknames: []

I initially found this error in flask_pyoidc:

Trace:

  File "/Users/akrug/workspace/sso-dashboard/env/lib/python2.7/site-packages/flask_pyoidc/flask_pyoidc.py", line 110, in _handle_authentication_response
    print(userinfo)
  File "/Users/akrug/workspace/sso-dashboard/env/lib/python2.7/site-packages/oic/oauth2/message.py", line 631, in __str__
    return '{}'.format(self.to_dict())
  File "/Users/akrug/workspace/sso-dashboard/env/lib/python2.7/site-packages/oic/oauth2/message.py", line 318, in to_dict
    elif isinstance(val, list) and isinstance(val[0], Message):
IndexError: list index out of range

Here is a version of the profile that threw the exception:

{
  "family_name": "Krug",
  "given_name": "Andrew",
  "nickname": "Andrew Krug",
  "groups": [
    "mozilliansorg_iam-project",
    "mozilliansorg_cloud incident response",
    "mozilliansorg_cis_whitelist",
  ],
  "emails": [
    {
      "verified": true,
      "name": "mozillians-primary",
      "value": "akrug@mozilla.com",
      "primary": true
    }
  ],
  "email": "akrug@mozilla.com",
  "name": "Andrew Krug",
  "picture": "https://cdn.mozillians.org/media/uploads/sorl-cache/92/2e/922efe09c26b1a04c93b7580ffae866d.jpg",
  "_HRData": {
    "placeholder": "empty"
  },
  "user_metadata": {},
  "lastName": "Krug",
  "userName": "akrug",
  "firstName": "Andrew",
  "displayName": "Andrew J Krug",
  "active": true,
  "shirtSize": "Fitted Small",
  "preferredLanguage": "en_US",
  "timezone": "US/Pacific",
  "primaryEmail": "akrug@mozilla.com",
  "lastModified": "2017-08-18T16:48:58+00:00",
  "created": "2017-02-06T22:53:54+00:00",
  "uris": [
    {
      "verified": false,
      "name": "mozillians-GitHub-39660",
      "value": "https://github.com/andrewkrug",
      "primary": false
    }
  ],
  "nicknames": [],
  "email_verified": true,
  "clientID": "7wyIItkJX4t7vYEaDmGrwP9k2fBh5qWP",
  "updated_at": "2017-08-21T19:15:36.519Z",
  "user_id": "ad|Mozilla-LDAP|akrug",
  "identities": [
    {
      "user_id": "Mozilla-LDAP|akrug",
      "provider": "ad",
      "connection": "Mozilla-LDAP",
      "isSocial": false
    }
  ],
  "created_at": "2017-02-06T22:53:30.473Z",
  "multifactor": [
    "duo"
  ],
  "sub": "ad|Mozilla-LDAP|akrug",
  "https://sso.mozilla.com/claim/groups": [
    "mozilliansorg_iam-project",
    "mozilliansorg_cloud incident response",
    "mozilliansorg_cis_whitelist",
  ],
  "https://sso.mozilla.com/claim/emails": [
    {
      "verified": true,
      "name": "mozillians-primary",
      "value": "akrug@mozilla.com",
      "primary": true
    }
  ],
  "https://sso.mozilla.com/claim/_HRData": {
    "placeholder": "empty"
  }
}

Here is an attempt I've made to do two things:

1. Make the statement more resilient. Return None instead of returning en exception.
2. Make the python more idomatic.

https://github.com/mozilla-iam/pyoidc/pull/1/files

Please let me know if you're interested in this PR and I'll open a pull to resolve this problem.

[decentral1se]

Thanks for the report @andrewkrug!

It may be acceptable to return None, I am not sure. Could you provide your motivation for not raising an exception? Also, a test would be very much appreciated as well.

@rohe might know what best course to take here, he added this in 1b7ad6c (which was 6 years ago - wow!). Please do open your pull request against this repository. We'll get it merged ASAP.

[andrewkrug]

Re: It may be acceptable to return None, I am not sure. Could you provide your motivation for not raising an exception? Also, a test would be very much appreciated as well.

Effectively this a really generic method on the message object that takes any number of items and converts them to a dictionary. The switch I've referenced on line 322 performs that isinstance comparison to determine if the to_dict() method needs to recurse further into the profile and continue turning things into dictionaries. In this case the statement I've provided in the below block isinstance(next(iter(val or []), None) simple allows you to pass a k,v to this method where v is an empty list. The expression elif isinstance(val, list) and isinstance(next(iter(val or []), None), Message): is evaluated to False and the fall through behavior _res[key] = val is called.

This allows RPs consuming ID Token and Userinfo data to receive empty lists back and have them parsed properly into sessions. The default behavior here for comparisons sake is still left intact as inside of this loop next(iter(val or []), None will always return the first item of a list for comparison to the Message provided of course that there is a first list item.

I'm happy to provide a test to cover this case OR alternatively I can extend your oic claims in the current body of tests to cover a case where one or more of the attributes is [] instead of "" or None.

Thanks

            if isinstance(val, Message):
                _res[key] = val.to_dict(lev + 1)
            elif isinstance(val, list) and isinstance(next(iter(val or []), None), Message):
                _res[key] = [v.to_dict(lev) for v in val]
            else:
                _res[key] = val

[decentral1se]

Thanks for explaining! Please add a test to cover this case and we're good! 🍻

[andrewkrug]

Do you know when we might see the next release of pyoidc with this in it?

Also @zamzterz will there be plans to bump flask-pyoidc to latest in support of having this bug fix in all the places?

[decentral1se]

I can make a minor release by monday if no one has objections. If you want to include the version bump, submit a PR (someone, anyone :)) and Ill review.

[andrewkrug]

@lwm less important for us at the moment as it would seem that the module we most care about only works with oic 0.9.1 ( flask_pyoidc ). I'm hoping that @zamzterz can commit to aligning that with the latest code.

I appreciate your willingness to do a dot release. Any chance we could get this into an 0.9.1.1 regression or something?

[zamzterz]

@andrewkrug Should definitely be possible to upgrade the dependency in Flask-pyoidc when a new release is done. I expect that it shouldn't require any large changes, but I'd appreciate help with testing if you're available.

[andrewkrug]

@zamzterz it would be great if you could give what's in the tip of master branch a try. When I attempted to upgrade I got mismatched issuer errors.

[decentral1se]

https://pypi.python.org/pypi/oic/0.11.0.1

🍻