fix: Respond with Redirect URL for already handled requests

[svrakitin]

Related issue

Closes #1569

Proposed changes

• Use 410 Gone instead of 409 Conflict for already handled requests
• Respond with redirect_to when getting Login/Consent/Logout requests which have already been handled.
• Make it more consistent by replacing WasUsed with WasHandled everywhere.

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 or changed the documentation.

[Benehiko]

Was it necessary to change everything from WasUsed to WasHandled? Other than that it looks good to me :)

[svrakitin]

Was it necessary to change everything from WasUsed to WasHandled? Other than that it looks good to me :)

@Benehiko I just made it more consistent as both WasUsed and WasHandled were mentioned in the code. I believe that request cannot really be used, it can be handled tho. :)

[Benehiko]

Really appreciate the contribution :)
I'll approve this, but will require further feedback from @aeneasr

[aeneasr]

Awesome, thank you! I've added a more descriptive comment. Can you please apply that comment to all the definitions of WasHandled?

Alternatively to this approach, we could do:

h.r.Writer().WriteError(w, r, x.ErrConflict.WithReason("Consent request has been used already").WithDetail("redirect_to", r.ReturnURL))

and write a custom swagger error object for this (so not genericError but e.g. requestWasHandledError.

What do you think? Is the 429 to counter intuitive? In my view, this is an error and should be treated as an error. If the dev does not check for this, the flow will end up in an irrecoverable error for the user.

Alternatively we could also let hydra redirect the user to the RequestURL if we encounter a used login / consent request?

[svrakitin]

@aeneasr I think replying with 409 Conflict on GET is not very intuitive as I would only expect conflict when mutating some state. If we still want to have an error here an alternative could be using different status code (i.e. 410 Gone) and still responding with redirect_to/request_url in a different swagger object as you mentioned. But maybe changing status code now will break someone.

[svrakitin]

@aeneasr Updated PR to keep 409, but respond with redirect_to.

[aeneasr]

410 Gone is a good idea!

[aeneasr]

True, it is a breaking change - but given that we fix the behavior which was basically completely broken before it is ok. We'll document it :)

[svrakitin]

Will make it 410 then! Thanks

[aeneasr]

Awesome @svrakitin !

[sneko]

Hi @svrakitin ,

Just upgraded my Hydra instance to benefit from your PR (thanks).

I noticed with my Go client I'm able to get the *admin.GetLoginRequestGone when running GetLoginRequest, which seems expected.

But when doing AcceptLoginRequest (I was doing only this one before when the user submit credentials), I get this type of error:
*runtime.APIError
(and once stringified it gives response status code does not match any response statuses defined for this endpoint in the swagger spec (status 409): {})

It brings to me some questions:

• It seems the 409 is still used for accepting a login?
• Should the Go client have the possible "Gone" error on AcceptLoginRequest as for GetLoginRequest? Or should I always do a Get before Accept just to make sure it's valid (considering in won't expire between both requests?

Thank you,

[svrakitin]

Hey @sneko,

I think it makes sense to revisit status codes for accept endpoints as well. For now as a workaround you can get login request first. 409 is used for accept, but it's not even mentioned in the swagger spec, thats why you get this generic error.

You can either submit a PR to fix that or I will find some time later this month.

[sneko]

@svrakitin just noticed that:

Some users started the flow into a specific browser, and then, due to confirmation through email... they went to another browser.

And even if they had a code challenge not used, GetLoginRequest was returning no Gone error, but the following AcceptLoginRequest was returning response status code does not match any response statuses defined for this endpoint in the swagger spec (status 409): {}.

I wonder what could make the difference so Hydra is complaining?

Let's see my code:

	getResponse, err := s.hydra.Admin.GetLoginRequest(&hydra_admin.GetLoginRequestParams{
		Context:        ctx,
		LoginChallenge: challenge,
	})
	if err != nil {
                 return err
	}

and

	acceptResponse, err := s.hydra.Admin.AcceptLoginRequest(&hydra_admin.AcceptLoginRequestParams{
		Context:        ctx,
		LoginChallenge: challenge,
		Body: &hydra_models.AcceptLoginRequest{
			Subject:     &auth.UserID,
			Remember:    true,
			RememberFor: int64(0), // Must be in seconds, and "0" for browser session lifetime
		},
	})
	if err != nil {
                 return err
	}

For me it's not coming from the missing Gone, I could still look for status 409 in the stringified error to adjust my frontend... but I don't get why the getter is a success if accepting is not :(

[svrakitin]

@sneko Could it be some race condition when you have multiple requests for the same challenge? It can be users refreshing/reusing the email confirmation page if you accept the challenge automatically there.

[sneko]

@svrakitin after investigating it's on our side. An error came after the return of Hydra so the user didn't get the redirectTo URL. Sorry about that!

Thanks for the help 👍 :)