openid-certification / oidctest

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

response mode is not strictly checked by OP tests #105

Open panva opened 6 years ago

panva commented 6 years ago

I've found the following issues when playing around with invalid response modes. It seems the response mode coming back from the OP is not strictly checked to be the one expected.

i.e.

higher prio, should be failing instead

lower prio, at least tests don't pass

Could the test suite imply the response_mode and check it strictly? For tests that i.e. force form_post or fragment this implied mode should be overwritten by whatever is specified in the test definition.

selfissued commented 6 years ago

Roland, can you fix this, starting with making sure that the two form post response mode tests verify that the responses actually use form post?

Also, do we know why the form post tests yields no results? We can't launch this profile until all the tests that are expected to work do.

Filip, thanks for identifying these problems.

panva commented 6 years ago

Also, do we know why the form post tests yields no results? We can't launch this profile until all the tests that are expected to work do.

Thats’s ok-ish, the tests that yield no results are the ones that expect fragment.

I’d really expect strict mode checks for every test.

selfissued commented 6 years ago

I'm confused. Isn't it legitimate, for instance, to use response_type=id_token and response_mode=form_post in a request and expect the ID Token to be returned in the form post body, rather than as a fragment? That should both work and be logged. Or are you describing something different, Filip?

I agree that we eventually want strict mode checks for every test. But as a first step, I'll settle for strict mode checks on the two form post tests, so we can get people started testing these new tests.

Do we know why the tool generates no log? There have been a few of those cases lately.

panva commented 6 years ago

Or are you describing something different, Filip?

I am.

When in any implicit or hybrid setup a form_post response yields no results (failure/pass/partial) and no logs.

What i mean by this is: when my instance is using code response_type and starts i.e. OP-Response-code but the OP responds with fragment then there are no logs and no results.

selfissued commented 6 years ago

@panva could you file a separate issue about the fact that if the OP responds with fragment values when using the code response type (and I'm guessing the code token response type too) that there are no logs?

@rohe can you please put in code so that OP-Response-form_post and the new negative test fail if the response doesn't use form post encoding? Hans is going to do a release Wednesday so hopefully this can be done before that. Thanks.

panva commented 6 years ago

@selfissued

and I'm guessing the code token response type too

code token is hybrid, query use is not allowed as per http://openid.net/specs/oauth-v2-multiple-response-types-1_0.html#Combinations

panva commented 6 years ago

I've tried the pushed changes by @rohe, here are the results

1) When an OP responds to form_post requests with a wrong response_mode i get this error message in the logs (in both Conditions section and Trace Output) authz_post:OP-Response-form_post: status=ERROR, message=Wrong HTTP method used and a PARTIAL_RESULT

Suggestion:

2) When an OP responds to tests expecting fragment with a query instead, i get this error message in the logs (only in Conditions section but missing from Trace Output) authz_post:OP-Response-id_token: status=ERROR, message=Wrong HTTP method used and a PARTIAL_RESULT

Suggestion:

3) When an OP responds to tests expecting query with a fragment instead, the test has no status and no logs

Suggestion: there should be logs, an ERROR and a proper error message consistent with the other cases.

edit: ERROR instead of a FAILURE.

rohe commented 6 years ago

There is nothing called FAILURE. ERROR is the as bad as it gets. I'll work with the other proposals.

panva commented 6 years ago

ERROR it is then. but not PARTIAL_RESULT (updated the above)

selfissued commented 6 years ago

Thanks for working on this, @rohe and for testing @panva . I believe the current state is good enough to use for the release so people can start testing the form post response mode tests. @zandbelt - please proceed to do the release. I agree with @panva that we eventually need to fix all the issues identified.

zandbelt commented 6 years ago

@panva I'm a bit lost on this, can you summarize the current status/request here? (for the record: the PR referred to above doesn't have anything todo with this ticket, that's just because of some weird crossreferencing across projects...)

panva commented 6 years ago

@zandbelt I will re-test.