feat: Adding form_post support #509

ajanthan · 2020-10-09T02:02:43Z

Related issue

#470

Proposed changes

Introduces Form attribute to AuthorizeResponse
Populates Form attributes in the relevant grant type handlers if the response_mode=form_post presents in request
Write the response as defined in the spec if the Form attributes are not empty

Checklist

I have read the contributing guidelines
I have read the security policy
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation within the code base (if appropriate)

Further comments

I haven't added support for response_mode= query or fragment or any other

ajanthan · 2020-10-09T03:24:43Z

cc @mitar @aeneasr

aeneasr

Thank you for your hard work on this and the tests and sorry for the late review but it's a bit of a complicated review :)

I think we can improve this PR a lot - it looks like there is a lot of duplication between AddForm and AddFragment. I think we should have a general ResponseMode() method instead of checking for ar.GetRequestForm().Get("response_mode") == "form_post". Since Form() and Fragment and Query are almost all very similar I think we could simplify this also and avoid duplication.

One question is how response_mode: form_post behaves when we're dealing with implicit flows. I couldn't find any guidance on this but to me it seems non-sensical to have implicit flows work together with form_post as implicit applications are typically javascript SPAs or mobile apps where you can't POST to. Do you have any guidance or sources on this?

authorize_helper.go

authorize_error.go

handler/oauth2/flow_authorize_code_auth.go

handler/oauth2/flow_authorize_implicit.go

handler/openid/flow_hybrid.go

…the initial handler and avoiding duplicate code in setting query,fragment and form parameter in authorize response

ajanthan · 2020-10-14T06:36:16Z

Thank you for your hard work on this and the tests and sorry for the late review but it's a bit of a complicated review :)

I think we can improve this PR a lot - it looks like there is a lot of duplication between AddForm and AddFragment. I think we should have a general ResponseMode() method instead of checking for ar.GetRequestForm().Get("response_mode") == "form_post". Since Form() and Fragment and Query are almost all very similar I think we could simplify this also and avoid duplication.

One question is how response_mode: form_post behaves when we're dealing with implicit flows. I couldn't find any guidance on this but to me it seems non-sensical to have implicit flows work together with form_post as implicit applications are typically javascript SPAs or mobile apps where you can't POST to. Do you have any guidance or sources on this?

No, I couldn't find any guidance around this. My thinking was if the response_mode is in the request process it accordingly otherwise process according to the default response mode for the flow. if it doesn't make sense to support form_post to the implicit flow, I don't mind it to restrict it to fragment only.

ajanthan · 2020-10-19T13:22:43Z

@aeneasr Did you get a chance to review the latest changes?

aeneasr

Sorry for the slow review, my week was extremely busy. Thank you for the hard work, I have some more ideas for improvement!

authorize_error.go

authorize_write.go

mitar · 2020-10-22T08:27:09Z

Do you have any guidance or sources on this?

So this is the spec for form post. In there it says:

Any technique supported by the User Agent MAY be used to cause the submission of the form, and any form content necessary to support this MAY be included, such as submit controls and client-side scripting commands. However, the Client MUST be able to process the message without regard for the mechanism by which the form submission was initiated.

Moreover, a non-normative request/response/request example they have in there does have response_type=id_token&response_mode=form_post in the authorization request. I think this is implicit flow, no? Where you directly get the id_token in the response which is form POSTed to the callback.

I must say that I do not understand why server simply does not do the form POST directly to the callback, why they have this JavaScript doing it? Is this done just so that any cookies to the callback URL which browser has go together with this form POST? Not sure why that would be needed, but this seems to be the spec.

I am not sure if it is really possible to talk about implicit/explicit flows here. I think from post is just its own beast. If implemented using the JavaScript post in the client, in fact it can work for SPAs as well if SPA implements a service worker which intercepts the POST (there are maybe other ways, too).

I think that the whole idea of response_mode is that it should work with any flow type because it is not really about the flow type. Flow type just configures the default response_mode. And one can override it. If it makes sense to override it for a particular flow type? We can leave this to the client.

mitar · 2020-10-22T08:30:12Z

I think it should be allowed for client to specify which response modes it allows. We should maybe have ResposneModeClient interface with GetResponseModes method?

authorize_request.go

mitar · 2020-10-24T03:35:35Z

authorize_request_handler.go

@@ -210,6 +210,23 @@ func (f *Fosite) validateResponseTypes(r *http.Request, request *AuthorizeReques
 request.ResponseTypes = responseTypes
 return nil
 }
+func (f *Fosite) validateResponseMode(r *http.Request, request *AuthorizeRequest) error {


This validation function should also respect security considerations from here:

There are security implications to encoding response values in the query string. The HTTP Referer header includes query parameters, and so any values encoded in query parameters will leak to third parties. Thus, while it is safe to encode an Authorization Code as a query parameter when using a Confidential Client (because it can't be used without the Client Secret, which third parties won't have), more sensitive information such as Access Tokens and ID Tokens MUST NOT be encoded in the query string. In no case should a set of Authorization Response parameters whose default Response Mode is the fragment encoding be encoded using the query encoding.

Also see description of combinations.

Based on that it is not really true that all combinations are possible. For example, if default uses fragment, then query cannot be used. Moreover, for errors, it seems that you can use always whichever the client requested.

So this is really that if by default query string is used, you can request that fragment is used. But not the opposite.

Given that for error any mode can be used, then maybe checking for valid combinations cannot be done here but in write handler?

Nice catch @mitar - so we should check for response_type and response_mode to define the output. It is also true that for errors, this check has to be disabled. But I also think that this has to be handled in the writer, because we first need to execute the "adapters" to figure out the default response mode and only then should we make assumptions about the default state.

Added the check for using query while the default mode is fragment and returns an error in the autheorize_response_writer.go.

authorize_response.go

authorize_helper.go

mitar · 2020-10-24T03:47:39Z

authorize_helper.go

+ <body onload="javascript:document.forms[0].submit()">
+ <form method="post" action="{{ .RedirURL }}">
+ {{ range $key,$value := .Parameters }}
+ <input type="hidden" name="{{$key}}" value="{{index $value 0}}"/>


Just for completeness (Because other encoding methods like query and fragment supports that), can you loop here over the value (which is a slice) and repeat <input with same key multiple times? This should never happen. But you never know if somebody directly modifies Parameters to add that (given that you defined it as url.Values which allows repeated values for same field).

Also, HTML-escape key and value here.

And add test for that.

html/template escapes all variables unless told otherwise.

Now the template supports adding multiple values. Also added test case for illegal character encoding.

aeneasr

Wow @ajanthan this is a really good PR! I think I found a couple more question marks and suggestions for improvement but we're very close to having this merged. Thank you for all the hard work, this is superb :)

authorize_helper.go

authorize_request.go

authorize_response.go

authorize_write.go

aeneasr · 2020-10-25T10:05:22Z

handler/openid/flow_hybrid.go

@@ -54,7 +54,9 @@ func (c *OpenIDConnectHybridHandler) HandleAuthorizeEndpointRequest(ctx context.
 if !(ar.GetResponseTypes().Matches("token", "id_token", "code") || ar.GetResponseTypes().Matches("token", "code") || ar.GetResponseTypes().Matches("id_token", "code")) {
 return nil
 }
-
+ if ar.GetResponseMode() == fosite.ResponseModeNone {


I think this should not run if fosite.ResponseModeQuery and maybe also not with the FormPost one. Please confirm @mitar . Also use ar.SetDefaultResponseMode.

added ar.SetDefaultResponseMode. Using query in this flow is already restricted but do we want to restrict form_post? According to the spec, it seems not necessary to restrict form_post as it more secure than query and fragment.

aeneasr · 2020-10-25T10:07:05Z

handler/openid/flow_hybrid_test.go

+ },
+ },
+ {
+ description: "Default responseMode check",


We should add a test for the concrete response modes and their expected results (e.g. error)

Agree. Added error test case in authorize_response_writer_test.go

aeneasr · 2020-10-25T10:07:13Z

handler/openid/flow_implicit.go

@@ -51,6 +51,10 @@ func (c *OpenIDConnectImplicitHandler) HandleAuthorizeEndpointRequest(ctx contex
 return nil
 }

+ if ar.GetResponseMode() == fosite.ResponseModeNone {


I think this should not run if fosite.ResponseModeQuery and maybe also not with the FormPost one. Please confirm @mitar . Also use ar.SetDefaultResponseMode.

Added ar.SetDefaultResponseMode. Using query in this flow is already restricted but do we want to restrict form_post?

aeneasr · 2020-10-25T10:07:33Z

handler/openid/flow_implicit_test.go

+ },
+ },
+ {
+ description: "default responseMode check",


Same as for the others, check for other response modes with expected success or errors.

+1 but it doesn't make sense to check success/error for other modes because the validation happens in authorize_response_writer. I have added a test case there.

Also added integration test to cover a various combination of response types and modes

aeneasr · 2020-10-25T10:08:44Z

integration/authorize_form_post_test.go

+ }{
+ {
+ description: "implicit grant test with form_post",
+ responseType: "token",


Should this be allowed @mitar ?

Changed it to id_token token.

fosite.go

aeneasr · 2020-11-02T23:10:36Z

Quick update from my side - this looks great! My plan would be to introduce an automated test pipeline using https://gitlab.com/openid/conformance-suite to ensure that all conformance tests pass as well. Unfortunately, this is quite the amount of work so it might need a bit of time before this is implemented. I have some other stuff to finish but this is next on my list afterwards!

aeneasr · 2020-11-03T10:55:29Z

I started working on this and already gotten pretty far (the set up works and the first tests are passing). I still have to figure out how to do this using automated tools (maybe I'll just use Cypress). But my feeling says this should be workable pretty soon!

mitar · 2020-11-03T12:57:14Z

I like Cypress. BTW, you can also check here how they did it.

aeneasr · 2020-11-03T13:07:31Z

Ah nice, I see how they solved the browser interaction. I was looking for this all night yesterday :D

aeneasr · 2020-11-03T13:59:16Z

I am making good progress here with the automation. Looks like almost everything is working as expected. Bad news is, the recent clean up of error_description actually breaks the conformity tests. But that should not block this PR!

I'll try to get all the tests working, then I will do a rewrite to this fosite branch with hydra and to check if form_post is implemented correctly. If it is, I will do another final review pass and then this is good to merge. Thank you for your patience!

aeneasr · 2020-11-03T13:59:36Z

mitar · 2020-11-03T14:17:15Z

Nice!

Bad news is, the recent clean up of error_description actually breaks the conformity tests.

Oh, what? Maybe open an issue with more information about this. This is surprising.

mitar · 2020-11-04T09:24:01Z

BTW, have you disabled some checks? Because I would assume #409 would fail the testing.

aeneasr · 2020-11-04T09:25:31Z

Certification works in such a way that you choose certain profiles. Also, not everything has to pass (e.g. none signature is not required to be implemented). So "orange" indicators are ok most of the times, black/red indicators are failures.

aeneasr · 2020-11-05T16:12:32Z

With a few fixes, most tests pass - there is one particular test suite which doesn't pass but I have some ideas why that could be. I will be working on this over the next days!

aeneasr · 2020-11-05T16:31:56Z

Ok so as far as I can tell the only thing that doesn't work is properly extracting the response_mode from the request object URL or JWT if one is set. We had this bug actually for state as well so I know where and what to fix. Other than that, I think this is pretty much good to go. Once I have confirmed the fix and ran all the tests I will give this one last code review and then I think we can merge this. Thank you for your work and commitment and patience @ajanthan . @vinckr please make sure @ajanthan gets some goodies from us :)

aeneasr · 2020-11-06T17:05:06Z

Please do not be confused by the random pushes, I am trying to debug something...

The PR itself is good to go IMO

mitar · 2021-04-09T05:37:13Z

authorize_request_handler_oidc_request_test.go

+ form: url.Values{"scope": {"openid"}, "response_type": {"code"}, "response_mode": {"none"}, "request": {validRequestObject}},
+ client: &DefaultOpenIDConnectClient{JSONWebKeys: jwks, RequestObjectSigningAlgorithm: "RS256"},
+ // The values from form are overwritten by the request object.
+ expectForm: url.Values{"response_type": {"token"}, "response_mode": {"post_form"}, "scope": {"foo openid"}, "request": {validRequestObject}, "foo": {"bar"}, "baz": {"baz"}},


Any particular reason why this file contains post_form and not correct form_post? What is this testing? Why it is not failing?

mitar · 2021-04-09T05:40:01Z

So changelog currently reads:

This patch introduces support for response_mode=form_post as well as response_mode of none and query and fragment.

I do not think this is correct. This PR did not introduce response_mode=none? That I think was just temporary implementation detail which was later renamed to "default" (so if response_mode is not provided). I think changelog should be probably fixed and mention of "none" removed?

mitar · 2021-04-11T18:16:46Z

Also, it seems this PR introduced a backwards incompatibility into fosite: if before this change, clients were providing response_mode, fosite ignored it and everything worked (if they were not asking for a non-default response mode). With just upgrading to a newer fosite (and when we use a custom Client implementation) such requests fail.

I see two solutions:

Client interface should simply get this new GetResponseModes method. After upgrading, compilation would fail, I would see what is missing, add it (based on default implementation), it is a simple change, and things will work as expected.
If client does not implement GetResponseModes, then response_mode parameter should simply be ignored, and not processed at all.

I think this maybe speaks more about fosite approach to backwards compatibility. I think it is great that compiler helps you point out what has to change to get things upgraded and I would prefer the first approach above. With a good message in the changelog I would know to expect this and would decide to do the upgrade with enough time budgeted to do the code change.

I think the introduction of a new interface makes things look like backwards compatible but in fact it was at the end much more work to discover the problem, get it reported internally, fix it, review the fix, and now me reporting it back here.

I know that I proposed a new interface but I was doing that based on prior practice in fosite. I think, @aeneasr, maybe this practice should be changed and we should embrace changing interfaces more instead of introducing new interfaces?

feat: Adding form_post support

27ea33c

aeneasr requested changes Oct 13, 2020

View reviewed changes

ajanthan added 2 commits October 13, 2020 22:56

refactor: introducing responseMode enum, setting responseMode during …

54a9d50

…the initial handler and avoiding duplicate code in setting query,fragment and form parameter in authorize response

Merge branch 'master' into feat-formpost

d359581

ajanthan requested a review from aeneasr October 14, 2020 06:26

aeneasr reviewed Oct 20, 2020

View reviewed changes

authorize_error.go Outdated Show resolved Hide resolved

authorize_write.go Show resolved Hide resolved

authorize_write.go Outdated Show resolved Hide resolved

authorize_write.go Show resolved Hide resolved

ajanthan added 2 commits October 20, 2020 12:44

refactor: Reusing response mode enum in error handling

ec3e000

fix: bulding authrize error message according to responseMode

0ebd04d

ajanthan requested a review from aeneasr October 21, 2020 04:58

mitar mentioned this pull request Oct 22, 2020

response_mode #1591 ory/hydra#1621

Closed