Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

/self-service/login/browser redirect fails with JSON response #368

Open
3 of 5 tasks
james-emerton opened this issue Feb 10, 2025 · 3 comments
Open
3 of 5 tasks

/self-service/login/browser redirect fails with JSON response #368

james-emerton opened this issue Feb 10, 2025 · 3 comments
Labels
bug Something is not working.

Comments

@james-emerton
Copy link

Preflight checklist

Ory Network Project

No response

Describe the bug

When Accept is application/json and the user already has a valid session the expected redirect response is malformed.

I'm using a UI based on the Ory Elements react-spa example. In the original code, refresh: true is always passed to createBrowserLoginFlow(), requiring the user to authenticate each time they attempt to authenticate via OIDC (with Hydra). When I remove refresh: true then Kratos will correctly detect the user is already authenticated and will attempt to redirect them to the returnTo URL. Unfortunately, when the request to create the flow is expecting a JSON response the response generated has status 200 and a body of null.

I believe this is due to the implementation of AcceptToRedirectOrJSON() function, which attempts to write the value of out (which is nil in this case) before calling http.Redirect(). Not having much experience with Go, it appears to me that the initial call to writer.Write() has the effect of sending a 200 response and causing http.Redirect() to be of no effect. (ie: 303 response is not sent and Location is not present in the response.)

I'm not certain what the correct behaviour here should be; when used as a JSON endpoint, redirecting to the returnTo address will likely not redirect the browser, implying that a JSON response would be more appropriate. It also seems that responding with 200 doing so could break the expectations of existing clients. It also seems to me that responding with an error (eg: session_already_available) could be misleading as the request did successfully complete the OAuth2 flow via hydra.AcceptLoginRequest.

Reproducing the bug

  1. Setup an environment with Kratos, Hydra, one of the -spa examples, and two or more OAuth2 client applications with skip_consent = True
  2. Modify the Login component to remove refresh from the call to createBrowserLoginFlow.
  3. Initiate an OIDC authorization code flow from a client application to establish a valid Hydra session.
  4. Initiate a second OIDC authorization code flow from a different client application.
  5. Observe that the response has status 200 and a body of null

Relevant log output

Relevant configuration

oauth2_provider:
  url: http://ory-hydra:4445

Version

1.3.1

On which operating system are you observing this issue?

Linux

In which environment are you deploying?

Docker

Additional Context

I've elected to use one of the SPA examples to avoid running another server process or having to re-implement the login endpoint in our (Python) application.

@james-emerton james-emerton added the bug Something is not working. label Feb 10, 2025
@aeneasr
Copy link
Member

aeneasr commented Feb 10, 2025

If you use accept: application/json, the response will always be json and never a browser redirect (since you requested json!)

@james-emerton
Copy link
Author

james-emerton commented Feb 10, 2025

@aeneasr I understand, but in that light how is the client (ie: react-spa) to know that the login request has succeeded and what URL it should redirect the browser to? The complicating factor is that I cannot access the relevant Hydra API directly from the browser to retrieve status and the redirect URL.

I believe I can work around this issue by implementing the login endpoint in my application, and given that I've already implemented the logout and consent endpoints it seems I should just give up and implement them all. 😄

At the very least, I feel the implementation of AcceptToRedirectOrJSON() could be more clear by not calling http.Redirect() when the negotiated content type is application/json.

@aeneasr
Copy link
Member

aeneasr commented Feb 11, 2025

Hm, I‘m not sure - a null body seems to be a bug but it could also just be an implementation issue of the SDK. Ideally a reproducible repo / example can be given as there are quite a lot of preconditions in the original issue, which makes it hard to understand what exactly is going on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working.
Projects
None yet
Development

No branches or pull requests

2 participants