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

/device should redirect to /device/verify #1416

Closed
benjaminleonard opened this issue Mar 22, 2023 · 5 comments
Closed

/device should redirect to /device/verify #1416

benjaminleonard opened this issue Mar 22, 2023 · 5 comments

Comments

@benjaminleonard
Copy link
Contributor

Currently it just shows the empty <AuthLayout /> template

@david-crespo
Copy link
Collaborator

david-crespo commented Mar 22, 2023

I was going to put up a PR, but I realized the redirect will not work yet when the console is served by Nexus because unlike the other routes, where we do things like /projects/* on the API side, we define the /device/verify handler directly, so /device will 404 server-side.

But that's actually fine, right? It's better than what you're seeing in local dev. We could fix it by defining /device server-side, but the whole question makes me wonder a couple of things:

  • In general, do we expect all prefixes of a give route to also exist? I'd say probably not. I don't think that's an expectation people have. So on that line of thinking, the problem here is the empty view — an actual 404 would be fine.
  • However, API 404s are ugly right now because they're just like a little JSON response, I think. This is an argument in favor of Allow overlapping route definitions dropshot#199, which would let us say ok /* is a console route, i.e., assume anything that doesn't have an explicit handler is a console route and we can serve the console and show the console's nice 404 for those. But we then also define /v1/* for API 404s.

@benjaminleonard
Copy link
Contributor Author

In general, do we expect all prefixes of a give route to also exist? I'd say probably not. I don't think that's an expectation people have. So on that line of thinking, the problem here is the empty view — an actual 404 would be fine.

Maybe? It feels like a decent practice, but I don't have the foresight to know if it'd cause any real issues.

An alternate solution might be to have it be a single route /device and handling verify and success as states within a single page.

@david-crespo
Copy link
Collaborator

We could also have server 404 give different responses based on whether the caller is asking for JSON, based on the Accept header.

oxidecomputer/dropshot#39

@benjaminleonard
Copy link
Contributor Author

Do we also want /device/success to be conditional on something in the header that does indeed show that you have verified successfully. Otherwise you could go their directly to get the serotonin boost of a device well authenticated without any of the commitment!

@david-crespo
Copy link
Collaborator

You can't really hit this route.

@david-crespo david-crespo closed this as not planned Won't fix, can't repro, duplicate, stale Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants