openid-certification / oidctest

THE CERTIFICATION TEST SUITE HAS BEEN MIGRATED TO A NEW SERVICE https://www.certificatinon.openid.net
Other
49 stars 15 forks source link

Silent failure when id_token returned as query parameter #150

Open andyMyersIBM opened 5 years ago

andyMyersIBM commented 5 years ago

If the OAuth OIDC provider returns an id_token as a query parameter, rather than as a fragment, in the Location header, then the test tool returns silently, providing no error or log information.

sozkan commented 5 years ago

@zandbelt I reviewed the code and I believe that this needs to be fixed in pyoidc. pyoidc parses the response and throws a generic error without providing any details about the root cause.

zandbelt commented 5 years ago

@tpazderka would you agree?

tpazderka commented 5 years ago

Could be. Do you have more details? Which particular test/endpoint causes this?

This will probably need a fix here as well, once the error is more specific/different, it might need to be caught properly, but that depends.

sozkan commented 5 years ago

Reproduction steps:

2019-05-02 13:55:18,725 otest.handling:ERROR [run_sequence] Exception: Missing or faulty response


Note the origin of the error `"/usr/local/lib/python3.6/dist-packages/oic-0.15.1-py3.6.egg/oic/oauth2/__init__.py", line 555`.
zandbelt commented 5 years ago

thanks @sozkan; @tpazderka I hope this helps to create an enhancement

tpazderka commented 5 years ago

Not sure about this. I think that the behaviour of pyoidc is correct in this case, it raises a ResponseError indicating that it cannot parse the response (as it is not where it is expected). The otest code is catching it and than throwing the exception in the logs:

https://github.com/openid-certification/otest/blob/825649302b2b986670a2fc888ccf859d7c9fc7aa/src/otest/aus/request.py#L327-L337

It looks like it should rather return None instead of the error instance, but that is probably a question for @rohe.

sozkan commented 5 years ago

@tpazderka if pyoidc can log the actual error reason (i.e using query parameters instead of fragment) that would be a significant improvement. Without that all we have is a generic "Missing or faulty response" at best.

tpazderka commented 5 years ago

Pyoidc cannot actually know that. It parses whatever is in the info variable and the test suite passes an empty string, so the best it can give you is a Missing response (that would require adding a check for empty string - could be added as an enhancement) but I am not sure if that would help you much.

If you want to be more specific in the error response, you can probably do some detection here: https://github.com/openid-certification/otest/blob/825649302b2b986670a2fc888ccf859d7c9fc7aa/src/otest/aus/request.py#L305-L318

sozkan commented 5 years ago

Thanks for the clarification @tpazderka. It looks it can be handled in otest before it reaches pyoidc as you suggested.

sozkan commented 5 years ago

@tpazderka I just changed it to return None in case of an error, see below: https://github.com/sozkan/otest/commit/15eaa0094560ebf472b02a9dc563b6e707094a10

After the above change, some tests started to return stack traces, so I made another change. https://github.com/sozkan/oidctest/commit/4a76abc847f60b2160b9c05afa8580130bbcd4a1

Now I have one more issue, now during VerifyResponse check last_protocol_response returns a RegistrationResponse, because I guess None is not added as a protocol response and last protocol response is the registration response. For example if you run OP-claims-auth_time-essential with the above changes, you get the following error which is not technically the right check.

verify-response: status=ERROR, message=Got a RegistrationResponse response !? [Checks that the last response was one of a possible set of OpenID Connect Responses]

Looks like a small change but it leads to more issues. I don't have a specific question but would appreciate any tips.