Change spoof login response back to 200 + JSON

[david-crespo]

This is 100% on me because I approved the 303 response after talking about it in great detail with @davepacheco, but the 303 is inconvenient for the console. The console makes the POST /login through the generated client like any other API call, and as such it freaks out if the response isn't JSON.

https://github.com/oxidecomputer/oxide.ts/blob/1c4561c83e352513c904c5c5d5b53b65adde88e0/static/http-client.ts#L106

We could try to accommodate this in various ways in the console itself or the generated client, but since AFAIK this endpoint exists solely for the console to use (and may disappear soon enough), it seems a lot easier to just change it back to an empty JSON response.

[david-crespo]

This was kind of a weird API, had to look at the Dropshot source to find a way that works. But I get the feeling what I'm trying to do here is a little unusual.

[davepacheco]

Ah, sorry for the breakage. Thanks for fixing it.

This is okay but I'm worried this solution leads to more confusion elsewhere... I'm not really sure so I'm thinking out loud here. It feels like the response codes for the login endpoints should be consistent, right? The SAML flow carries through state about where you were coming from so that the server can redirect you back there after login. I think that means saml_login has to return a 300-level response. I guess that doesn't have the same problem as spoof_login because the saml_login request is triggered by the IdP and not the web console, so saml_login has to send a 300 just to get you to the web console, whereas something like spoof_login could in principle do the redirect on the client. (Does it already do that?) We'll have the same issue when we shortly add password login. (I assume the password login page will be part of the console.) I had assumed that these would eventually all support the same kind of redirect and issue 303s on success. But maybe saml_login is just different from the other two and it's okay if they're not consistent with each other?

If we do want to change to a 200-level code, would "204 No Content" (with an empty response body) work? We do that for some other endpoints so I'd expect clients to be okay with this. It seems cleaner than a 200 with a special type that happens to be empty.

We could also return a 300 (with "Location" header) and still provide a JSON response body. I didn't do that because I wasn't sure what information we'd want to put in the response that's not already available in the status code and Location header. But HTTP encourages a response body and we can do that. But that raises the question: should the generator just handle 303s with no response better (i.e., similar to what it does for 204s)?

What do you think?

[david-crespo]

204 makes sense. I think I just forgot that was an option. I will make that change.

I think these login endpoints are different for the reasons you give. The SAML login one is where you land after IdP login and we need it to do an actual browser redirect, so the 303 is appropriate there. But spoof login and username/password login are AFAIK going to be hit only as background API calls by the console client. So while, like I said, we certainly could make the generated client treat the 303 as a success and ignore its contents otherwise, I'm not sure it's worth doing because a) this would be the only endpoint we do that for, and b) I don't know of a situation where that 303 is going to be used properly as a 303 (i.e., let the browser handle it and follow the redirect). If you have a 303 that's always meant to be ignored/have its handling suppressed, to me that's at least as confusing as a lack of consistency among the login endpoint responses.

On the other hand, if instead of simply pretending in the client that the 303 is a 204, we treat it as success and use the Location header to send the user someplace, then it could make a bit more sense to use a 303. That redirect on login success is currently done through the state query param and it's pretty simple and it works, so I'm reluctant to change it. I would also note that even if we do that thing with the location, we're not "following" the redirect the normal way, so we'd still need to do some shenanigans on the fetch to get it to not follow it:

https://fetch.spec.whatwg.org/#concept-request-redirect-mode

By default, in follow mode, it follows the redirect in the background — the target page is fetched, but the browser doesn't navigate there.