Serve unauthed console page at /login/{silo_name}/local#3217
Conversation
| .await | ||
| .expect("failed to get login form"); | ||
| let unauthed_console_paths = | ||
| &["/spoof_login", "/login/irrelevant-silo/local"]; |
There was a problem hiding this comment.
The changes in this file are mostly cleanup. This line is the only substantive change: check the new path in addition to /spoof_login.
| // apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await | ||
| serve_console_index(rqctx.context()).await | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not sure this endpoint will need to exist after we have silo in the domain. I was thinking a single GET /login endpoint could show either username/password login or a link to the IdP as appropriate depending on the silo. Because this is all done through the console, the generic console page be served as usual, and then the console would have to make an API call to determine which thing to show.
There was a problem hiding this comment.
It'd be nice if we could avoid having to make an extra API call by including the silo in the header or something. Not for this PR, but in whatever solution we ultimately end up with.
There was a problem hiding this comment.
Yeah, it really would. I think we might soon get to the point where doing that is less ridiculous than the stuff we're doing to get around it. On the other hand, the total decoupling of the frontend from the API has been very beneficial to my sanity so far.
| path = "/login/{silo_name}/local", | ||
| path = "/v1/login/{silo_name}/local", |
There was a problem hiding this comment.
I intentionally left the /v1/ off the console endpoints which the thought being that they'd need to be relatively stable. I can see wanting to gate the POST endpoint though given that it'll mostly be treated like any other API endpoint.
There was a problem hiding this comment.
Yeah, I went back and forth. It is a JSON API endpoint, so I figured it should probably follow the convention. I'm thinking we should get to a point where only the GETs don't have v1 and they're all unpublished. It feels weird to have prefixed and non-prefixed endpoints right next to each other, but that's where I landed as least unreasonable.
There was a problem hiding this comment.
Like above, will this endpoint not exist in a subdomain world?
There was a problem hiding this comment.
I'm not sure. We could have POST /v1/login just always be username/password, and that would make this endpoint unnecessary. Or maybe we could have POST /v1/login/local and we'd change POST /login/{silo_name}/saml/{provider_name} to POST /v1/login/saml/{provider_name}?
There was a problem hiding this comment.
The latter part feels better than me. Given that username/password log ins should be relatively rare I don't know that I'd want it to be the root endpoint (which makes it feel like the default).
There was a problem hiding this comment.
Yeah, I think that's right.
|
😅 |
* basic username/password login page * extract shared code * fix comment * regen API with oxidecomputer/omicron#3217 * bump omicron version to an actual omicron main commit
A version of #3203 where I keep it simple instead.
/login/{silo_name}/localwithout auth check/v1prefix toPOST /login/{silo_name}/localandPOST /logoutPOST /login/{silo_name}/localsuccess response from a 303 redirect to a 204redirect: manualinfetch()call to prevent it following the redirect. This does seem to work, but based on the spec it really seems like you're not supposed to do this. The resulting response is called an opaque-filtered redirect response and is not supposed to be good for anything but following the same redirect lateroxide.tsto not error out on HTML. That would be here. It's feasible but it would complicate things and it doesn't seem worthwhile.