fix: allow retries of accept login request #2914

jonkjenn · 2022-01-03T13:50:42Z

Related issue(s)

#2824

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code 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 or changed the documentation.

Further Comments

CLAassistant · 2022-01-03T13:50:49Z

All committers have signed the CLA.

aeneasr

Happy new year @jonkjenn :)

Thank you for working on this issue and your PR! I have two asks of you:

Would it be possible to make your changes against the feat: implement Ory Hydra v2.0 #2796 branch (in particular PR feat: introduce the concept of a flow in the persistence layer #2836) as we are changing the database layout.
As described in issue Login request not fully marked as used until browser navigates to /oauth2/auth #2824 we originally wanted to return 429 with an URL the developer can use for further propagation. The reason for not allowing double submit here is that we want to prevent conflicting submissions and potential race conditions. It's been quite some time that we introduced this and I don't quite remember the reasoning. But I think we need a sound overview of the potential attack vectors of allowing double submission here before we can change the behavior to just override the previous request or ignore it.
Can you introduce this feature to both handling consent and login?

aeneasr · 2022-01-03T13:59:38Z

persistence/sql/persister_consent.go

 		if err != nil {
 			return sqlcon.HandleError(err)
 		}

+		if !found {


This would only work in the case of an exact retry. It will be confusing for developers who expect the second call to override the first one, which is why we wanted to return 429 with an URL the developers can use for next action.

I've refactored it to the 2.x branch. You now get the 409 redirect_to. If it's conceptually ok I can add it to the consent step also.
I would maybe have chosen to use a specific error instead of x.ErrConflict so can change that if the approach seems ok otherwise.

Looks like consent is supported already #2914 (comment). Do you see the need for further work to ensure that enabling this at all is a secure solution @aeneasr? I.e. is it not realistic to get a change like this merged as more of a quick fix?

codecov · 2022-01-03T14:31:39Z

Codecov Report

❗ No coverage uploaded for pull request base (v2.x@10a3cd7). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 307dc9d differs from pull request most recent head 022fef3. Consider uploading reports for the commit 022fef3 to get more accurate results

@@           Coverage Diff           @@
##             v2.x    #2914   +/-   ##
=======================================
  Coverage        ?   76.32%           
=======================================
  Files           ?      107           
  Lines           ?     7426           
  Branches        ?        0           
=======================================
  Hits            ?     5668           
  Misses          ?     1349           
  Partials        ?      409

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10a3cd7...022fef3. Read the comment docs.

jonkjenn · 2022-01-10T14:03:16Z

Can you introduce this feature to both handling consent and login?

It already seems to work to retry the accept consent requests. I tested v1.10.6 towards our project and this branch by modifying the Cypress e2e tests with duplicate requests and also some unit test but those gets quite convoluted quickly when you get to the consent step since there's so much setup and options.

mig5 · 2022-04-01T00:40:18Z

This looks great and as far as I can tell, would make the behavior consistent with Login Requests when the token is attempted to be reused?

I'm not sure there's a security risk associated if it means that via the redirect_uri, the browser can reattempt the auth flow. Presumably the token would be different once it arrives at the consent screen again as part of that auth flow.

Would love to see this in the next release!

aeneasr · 2022-04-10T14:14:24Z

@jonkjenn is this ready for review? :)

jonkjenn · 2022-04-11T07:19:49Z

I changed to returning 200 on retries to stay consistent with how accept consent works also it feels more correct not to use an error for this case and it's simpler but could easily switch back to 409. The new tests shows more clearly how retries of both accept login and accept consent works.

aeneasr · 2022-04-17T19:19:28Z

Thank you! I think the tests are awesome! I would like to follow up with a comment made in this issue:

#2824 (comment)
#2824 (comment)
#2824 (comment)

which would basically mean that we return an error like 409/410 but include a redirect_url in the response:

{"error":"Unable to insert or update resource because a resource with that value exists already","error_description":"","redirect_to":"https://.../oauth2/..."}.

which would help the developer implementing this flow to identify the URL they need to return the user to. What do you think about that solution? In the issue thread, it appears that most agree :) I think it would be relatively straight forward to modify this PR to follow that behavior as all the tests and everything is already in place :)

jonkjenn · 2022-04-19T12:44:42Z

@aeneasr So 409 for /accept/login and leave /accept/consent as 200?

aeneasr · 2022-04-19T14:07:09Z

409 for both - or is consent currently returning 200 on conflict? Sorry I didn’t have time yet to refresh my knowledge on the issue…

mig5 · 2022-04-20T03:59:09Z

@jonkjenn @aeneasr

So 409 for /accept/login and leave /accept/consent as 200?

No

409 for both - or is consent currently returning 200 on conflict? Sorry I didn’t have time yet to refresh my knowledge on the issue…

Yes (409 for both, but more importantly, redirect_to parameter in both the 409 error responses)

Let me try and clarify:

Both Login and Consent flows can experience a '410 Gone' on GET /oauth2/auth/requests/login?login_challenge=xxxxxxxxxxxxx or GET /oauth2/auth/requests/consent?consent_challenge=xxxxxxxxxxxxxxx

In both cases, the 410 Gone error has a redirect_to parameter in the error response, which we can redirect to in order to 'restart' the flow transparently without the user getting blocked on an error page.

But a 409 Conflict on the Consent flow (PUT /oauth2/auth/requests/consent/accept?consent_challenge=xxxxxxxxxxxxxx) does not have a redirect_to in the response. According to #2824 (comment) , this is also true for the 409 on the Login flow (but I personally haven't been able to trigger that scenario, only the 410)

In other words, in both Login and Consent flows, a PUT with the challenge token throws a 409 but does not have a redirect_to in the error response. But a 410 Gone (on the GET) does.

We are hoping for the same redirect_to parameter to be added to the error response for a 409, in either Login or Consent case. That's all.

The 409 Conflict on the Consent form is easy to trigger, assuming you are not 'skipping' the consent:

get to the 'Consent' form in a tab (after completing the Login flow), e.g https://yourloginandconsentapp.com/consent?consent_challenge=xxxxxxxxxxxxxxxxxxx
load that same page in a second tab (it's got the same consent_challenge token)
submit the form on the first tab (flow completes normally)
submit the form on the second tab (gets the following 409 Conflict in Hydra):

time=2022-04-20T03:35:00Z level=error msg=An error occurred while handling a request audience=application error=map[debug: message:Conflict reason:The consent request was already used and can no longer be changed. status:Conflict status_code:409] http_request=map[headers:map[accept:application/json content-length:130 content-type:application/json [.......] method:PUT path:/oauth2/auth/requests/consent/accept [.....] http_response=map[status_code:409] service_name=Ory Hydra service_version=v1.11.7

... and there is no redirect_to in the error response unlike a 410 :

Apr 20 03:47:38 login.xxxxxxx.xxxx php: ERROR: [410] Client error: `GET https://xxxxxx.xxxxxxxx.xxxxxx/oauth2/auth/requests/login?login_challenge=xxxxxxxxxxxxx` resulted in a `410 Gone` response:#012{"redirect_to":"https://xxx.xxxxxxx.xxxx/oauth2/auth?claims=null\u0026client_id=xxxxxxx (truncated...)

(Obviously if you optionally 'save' the consent the first time, you tend not to hit this problem again (at least until any such consent is revoked by the user), because the redirects happen automatically and the Consent form thus never has to be submitted again.)

Ultimately the most important thing from an implementor's perspective is to be able to gracefully recover by consuming the redirect_to in the error response for any error. Right now that only works for 410s but not the 409 on PUT.

I hope that clarifies things!

mig5 · 2022-04-20T04:02:59Z

Edited my comment above to note that it's named redirect_to in the 410 Gone error responses, not redirect_uri. We obviously would want this to be consistently named in the 409 too once it exists for that response :)

jonkjenn · 2022-04-20T07:24:37Z

@mig5 You're talking about the case where the user has visited the the redirect_to returned from the first /consent/accept as I understand it? Just to be clear this PR does not attempt to deal with that scenario only the scenario where you "need to" perform multiple /(login|consent)/accept BEFORE the user visits the redirect_to.

All my tests indicate that before the user visits the redirect_to in the consent case the response to multiple /consent/accept requests is always 200 with the same response payload. This test hopefully shows it directly https://github.com/ory/hydra/pull/2914/files#diff-852c558f6e189a7d5ce22301e754abf3ef10d2785b7a708922cbd026413009c4R101 and it's what I see when I manually verify.

@aeneasr

409 for both - or is consent currently returning 200 on conflict? Sorry I didn’t have time yet to refresh my knowledge on the issue…

Yes it is currently returning 200 with the same redirect_to payload on multiple PUT consent/accept requests BEFORE the user has visited the redirect_to. Which is why it feels strange to have different behavior for PUT login/accept unless it's really required. Changing the consent response of course would be a breaking change.

mig5 · 2022-04-20T07:29:31Z

@jonkjenn

only the scenario where you "need to" perform multiple /(login|consent)/accept BEFORE the user visits the redirect_to.

Right - I am saying, when the 'multiple' request fails on 409 (PUT), it would be nice to have the same redirect_to be present in the error message, for both Login and Consent. Right now in the latest release of Hydra, there is only a redirect_to on 410 (a failure on GET), but not on the 409.

aeneasr

@grantzvolsky can you maybe take a look, since you wrote the state machine? Does the code change make sense in your view?

aeneasr · 2022-04-22T19:15:57Z

flow/flow.go

+	if f.State == FlowStateLoginUnused {
+		return nil
+	} else if f.State != FlowStateLoginInitialized {


Would it make sense to use the same logic as in the consent flow?

if f.State != FlowStateConsentInitialized && f.State != FlowStateConsentUnused && f.State != FlowStateConsentError { return errors.Errorf("invalid flow state: expected %d/%d/%d, got %d", FlowStateConsentInitialized, FlowStateConsentUnused, FlowStateConsentError, f.State) }

On a second look I am still confused though. The 409 error does not happen in this logic here, it comes from the database or explicitly returning x.ErrConflict.

I then executed the tests included in this PR without the changes above, and the tests pass, indicating that the change has not the desired effect and the code on this branch already behaves the way we want it to?

if f.State != FlowStateLoginInitialized && f.State != FlowStateLoginUnused { return errors.Errorf("invalid flow state: expected %d/%d, got %d", FlowStateLoginInitialized, FlowStateLoginUnused, f.State) }

I don't understand exactly what you're saying is working or not but the suggestion of changing it to be more similar to the HandleConsentRequest function makes sense to me. You seem to get some extra consistency validation compared to just jumping out.

Thank you for getting back :) I ran the tests you provided but changed the code in flow.go back to what it was before your changes, and the tests you wrote still pass, making me wonder whether we need this change in flow.go?

Hmm they fail locally for me but I could be doing something wrong. I created #3085 to attempt to run tests in the pipeline but if there is a PR pipeline I didn't manage to trigger it.

My bad @jonkjenn ! You're right :)

@jonkjenn cc @aeneasr you are right that there are some opportunities for more consistency checks. When I introduced Flow, I initially tried to validate everything. But it turned out that some tests depend on imperfections in the validation, and that updating them would be a lot of work, so I changed my strategy and just made the minimum necessary changes.

aeneasr · 2022-04-22T19:25:16Z

flow/flow.go

+	if f.State == FlowStateLoginUnused {
+		return nil
+	} else if f.State != FlowStateLoginInitialized {


On a second look I am still confused though. The 409 error does not happen in this logic here, it comes from the database or explicitly returning x.ErrConflict.

I then executed the tests included in this PR without the changes above, and the tests pass, indicating that the change has not the desired effect and the code on this branch already behaves the way we want it to?

aeneasr · 2022-04-22T19:26:19Z

Thank you all for the explanations! As per my review, it appears that the code is already behaving as expected in the test cases as they seem to pass without the changes to flow.go.

Closes #2914 Closes #3085 See #2824

jonkjenn · 2022-04-25T08:08:38Z

Oops didn't notice your PR

aeneasr · 2022-04-25T14:00:13Z

Sorry about that @jonkjenn - I should have linked it here! I basically used all of your code but restructured the logic a bit in the handler :) Getting the tests to pass now and then it's good for merge! Thank you for the hard work and sorry for the long turn-around!

Closes #2914 Closes #3085 See #2824

aeneasr requested changes Jan 3, 2022

View reviewed changes

jonkjenn force-pushed the fix__allow_retries_of_accept_login_request branch from a1469e4 to c0f3843 Compare January 5, 2022 06:32

jonkjenn changed the base branch from master to v2.x January 5, 2022 07:12

jonkjenn force-pushed the fix__allow_retries_of_accept_login_request branch from b734c5e to 534d128 Compare January 5, 2022 07:28

jonkjenn requested a review from aeneasr January 5, 2022 09:24

jonkjenn force-pushed the fix__allow_retries_of_accept_login_request branch from 460079b to 50bec3f Compare March 24, 2022 08:05

jonkjenn marked this pull request as ready for review April 11, 2022 07:19

aeneasr reviewed Apr 22, 2022

View reviewed changes

aeneasr requested changes Apr 22, 2022

View reviewed changes

aeneasr approved these changes Apr 24, 2022

View reviewed changes

aeneasr force-pushed the v2.x branch from 0df9e6e to 10a3cd7 Compare April 24, 2022 20:44

aeneasr pushed a commit that referenced this pull request Apr 24, 2022

fix: allow retries of unused login & consent requests

ca37129

Closes #2914 Closes #3085 See #2824

aeneasr mentioned this pull request Apr 24, 2022

fix: allow retries of unused login & consent requests #3088

Merged

7 tasks

jonkjenn added 3 commits April 25, 2022 09:46

Refactor

bbda6f5

Refactor

ffa938b

Refactor

1639982

jonkjenn added 9 commits April 25, 2022 09:46

Refactor

db7782b

Refactor

cefd865

Cleanup

0eef707

Rebase

d7c2edf

Refactor

77a1358

Refactor

2f67966

Refactor

aaa0523

Cleanup

30209b7

Return 200 add tests

022fef3

jonkjenn force-pushed the fix__allow_retries_of_accept_login_request branch from e4c89e4 to 022fef3 Compare April 25, 2022 07:47

jonkjenn closed this Apr 25, 2022

aeneasr pushed a commit that referenced this pull request Apr 27, 2022

fix: allow retries of unused login & consent requests

536d5fc

Closes #2914 Closes #3085 See #2824

aeneasr pushed a commit that referenced this pull request Apr 27, 2022

fix: allow retries of unused login & consent requests

369403e

Closes #2914 Closes #3085 See #2824

grantzvolsky pushed a commit that referenced this pull request May 19, 2022

fix: allow retries of unused login & consent requests

7f72bfe

Closes #2914 Closes #3085 See #2824

aeneasr pushed a commit that referenced this pull request Jun 27, 2022

fix: allow retries of unused login & consent requests

08df1a1

Closes #2914 Closes #3085 See #2824

grantzvolsky pushed a commit that referenced this pull request Aug 1, 2022

fix: allow retries of unused login & consent requests

d19c7b5

Closes #2914 Closes #3085 See #2824

aeneasr pushed a commit that referenced this pull request Aug 1, 2022

fix: allow retries of unused login & consent requests

37d0e91

Closes #2914 Closes #3085 See #2824

aeneasr pushed a commit that referenced this pull request Aug 18, 2022

fix: allow retries of unused login & consent requests

6f8f9cc

Closes #2914 Closes #3085 See #2824

aeneasr pushed a commit that referenced this pull request Sep 5, 2022

fix: allow retries of unused login & consent requests

a327106

Closes #2914 Closes #3085 See #2824

aeneasr pushed a commit that referenced this pull request Sep 7, 2022

fix: allow retries of unused login & consent requests

51a586b

Closes #2914 Closes #3085 See #2824

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow retries of accept login request #2914

fix: allow retries of accept login request #2914

jonkjenn commented Jan 3, 2022 •

edited

CLAassistant commented Jan 3, 2022 •

edited

aeneasr left a comment

aeneasr Jan 3, 2022

jonkjenn Jan 5, 2022

jonkjenn Jan 12, 2022

codecov bot commented Jan 3, 2022 •

edited

jonkjenn commented Jan 10, 2022

mig5 commented Apr 1, 2022

aeneasr commented Apr 10, 2022

jonkjenn commented Apr 11, 2022 •

edited

aeneasr commented Apr 17, 2022

jonkjenn commented Apr 19, 2022

aeneasr commented Apr 19, 2022

mig5 commented Apr 20, 2022 •

edited

mig5 commented Apr 20, 2022

jonkjenn commented Apr 20, 2022

mig5 commented Apr 20, 2022

aeneasr left a comment

aeneasr Apr 22, 2022

aeneasr Apr 22, 2022

jonkjenn Apr 23, 2022 •

edited

aeneasr Apr 23, 2022

jonkjenn Apr 23, 2022

aeneasr Apr 24, 2022

grantzvolsky Apr 25, 2022

aeneasr Apr 22, 2022

aeneasr commented Apr 22, 2022

jonkjenn commented Apr 25, 2022

aeneasr commented Apr 25, 2022

fix: allow retries of accept login request #2914

fix: allow retries of accept login request #2914

Conversation

jonkjenn commented Jan 3, 2022 • edited

Related issue(s)

Checklist

Further Comments

CLAassistant commented Jan 3, 2022 • edited

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 3, 2022 • edited

Codecov Report

jonkjenn commented Jan 10, 2022

mig5 commented Apr 1, 2022

aeneasr commented Apr 10, 2022

jonkjenn commented Apr 11, 2022 • edited

aeneasr commented Apr 17, 2022

jonkjenn commented Apr 19, 2022

aeneasr commented Apr 19, 2022

mig5 commented Apr 20, 2022 • edited

mig5 commented Apr 20, 2022

jonkjenn commented Apr 20, 2022

mig5 commented Apr 20, 2022

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkjenn Apr 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Apr 22, 2022

jonkjenn commented Apr 25, 2022

aeneasr commented Apr 25, 2022

jonkjenn commented Jan 3, 2022 •

edited

CLAassistant commented Jan 3, 2022 •

edited

codecov bot commented Jan 3, 2022 •

edited

jonkjenn commented Apr 11, 2022 •

edited

mig5 commented Apr 20, 2022 •

edited

jonkjenn Apr 23, 2022 •

edited